fix: resolve type errors/add type annotations #151
Conversation
|
|
||
|
|
||
| def translate_db2api(namespace, alias): | ||
| def translate_db2api(namespace: str, alias: str) -> list[tuple[str, Optional[str]]]: |
There was a problem hiding this comment.
Might be good to create an enum for namespace in a separate issue (if it doesn't already exist)
There was a problem hiding this comment.
Maybe -- I think that makes usage of the store function complicated because users might want to add variations under arbitrary/custom namespaces
There was a problem hiding this comment.
That makes sense. Maybe just an enum for the sources that are used frequently in seqrepo?
There was a problem hiding this comment.
Yeah, or maybe a predefined union of constants? otherwise you have to annotate everything as namespace_enum | str and handle both kinds of types every time
| @@ -55,14 +56,14 @@ def translate_api2db(namespace, alias): | |||
| return [ | |||
| ("VMC", "GS_" + alias if alias else None), | |||
| ] | |||
| if namespace == "ga4gh": | |||
| if namespace == "ga4gh" and alias is not None: | |||
There was a problem hiding this comment.
Is there a reason why we don't just return an empty list at the beginning if alias is none?
There was a problem hiding this comment.
Yeah, I had a hard time with this and have gone through it again. The call stacks that end up in this method are kind of messy, but I think this is the reason for the way it is right now:
-
storeand its descendant functions shouldn't ever be passingNonefor an alias because it's a non-nullable field within the DB schema -
find_aliasesusesNonefor all fields as a wildcard indicator. So if I call this method with("refseq", None), that means I want to be running a search for all aliases with namespace"NCBI"(if we return an empty list ifaliasisNone, then this lookup would be erroneously turned into "get all aliases regardless of namespace").
The change here is to ensure that we don't get a TypeError on the next line if alias is None. Given the structure of the 3 lines above this condition, I'm not sure if it should look like
if namespace == "ga4gh" and alias is not None:
return [
("VMC", "GS_" + alias[3:]),
]
or
if namespace == "ga4gh":
return [
("VMC", "GS_" + alias[3:] if alias else None),
]
either way, I updated the typing on the store_alias side of the call stack above this method to ensure that it doesn't permit alias to be None on its end -- but it does need to be able to return a nonempty list even when alias is None sometimes
| """Creates a new sequence repository if necessary, and then opens it""" | ||
|
|
||
| self._root_dir = root_dir | ||
| self._db_path = os.path.join(self._root_dir, "db.sqlite3") | ||
| self._writing = None | ||
| self._db = None |
There was a problem hiding this comment.
Same comment as other PR, should this be separated out in a different issue?
There was a problem hiding this comment.
I sort of intended this PR to be that issue. This is a line that doesn't do anything except establish a flawed type annotation that becomes problematic for how the entire rest of the class is written. I originally addressed it in the other issue only because its existence makes pyright (checked as part of precommit) explode if you touch anything else in this module, but the only reason it needs to change at all is for type checking reasons, not because it affects function (since the variable is reassigned to a real value four lines down).
There was a problem hiding this comment.
Ah okay. I didn't pull anything up in my editor so couldn't see any warnings and it just appeared that you were deleting unused code since you were already in that module
No description provided.