Update all annotations and check with Mypy #520
Conversation
| organized_messages: dict[str, Union[dict, list[InspectorMessage]]], | ||
| organized_messages: dict, |
There was a problem hiding this comment.
While this could probably be typed with more detail as was attempted originally, it turns out to be pretty tricky to do right so maybe someone can tackle that later on its own PR
| if isinstance(neurodata_object, NWBFile): | ||
| return neurodata_object.container_source | ||
|
|
||
| return neurodata_object.get_ancestor("NWBFile").container_source # type: ignore |
There was a problem hiding this comment.
This type of ignore is a potentially interesting one - it's because I type a generic NWB-type object as a Python object so any of the special class methods like .get_ancestor naturally are more specific than that - so it would be cool to have a common base neurodata object in PyNWB that all types inherit from (does that already exist? For both datasets and their containers / groups?)
There was a problem hiding this comment.
(recommend making more specific typing improvements down the road, and when ready remove the ignores here)
There was a problem hiding this comment.
There is the AbstractContainer class inherited from by both Container and Data types, but I don't know if there is a common base neurodata object specifically...
Definitely worth discussing for typing improvements later on
|
@stephprince OK, sorry this one has quite a bit of a changelog (just a lot of the same things) - I tried excluding it piece by piece by the exclusion rules were not playing nice with pre-commit so went ahead and did it all |
There was a problem hiding this comment.
If I understand correctly, PathType and FilePathType are still being used in a couple of places, but once I update those they should be able to be deprecated?
There was a problem hiding this comment.
Yep that sounds like a plan, I just did what I had to do to make Mypy happy, which was to sometimes fix that and other times not
| local_path = Path(testing_config["LOCAL_PATH"]) | ||
| local_path.mkdir(exist_ok=True, parents=True) | ||
| testing_folder = Path(TESTING_FILES_FOLDER_PATH) | ||
| testing_folder.mkdir(exist_ok=True) |
There was a problem hiding this comment.
| testing_folder.mkdir(exist_ok=True) | |
| testing_folder.mkdir(exist_ok=True, parents=True) |
might still be needed if nested directories?
There was a problem hiding this comment.
I caution against doing that very stringly, because if a target path is formatted incorrectly by mistake (say, like a WindowsPath) then calling parents=True will cause that folder to be created even if the root makes no sense for the OS (such as UNIX)
Ran into that one a couple times before.
If you want to ensure specific parents exist, I suggest going through them one at a time to make sure they exist (from top to bottom of course)
| if isinstance(neurodata_object, NWBFile): | ||
| return neurodata_object.container_source | ||
|
|
||
| return neurodata_object.get_ancestor("NWBFile").container_source # type: ignore |
There was a problem hiding this comment.
There is the AbstractContainer class inherited from by both Container and Data types, but I don't know if there is a common base neurodata object specifically...
Definitely worth discussing for typing improvements later on
fix #416, at long last