Fix type hints for callbacks, utils and VecTranspose
#1648
Conversation
araffin
requested review from
ernestum,
AdamGleave,
Miffyli,
hill-a,
andregonz and
qgallouedec
| @@ -255,7 +260,7 @@ def open_path_pathlib(path: pathlib.Path, mode: str, verbose: int = 0, suffix: O | |||
|
|
|||
| if mode == "r": | |||
| try: | |||
| path = path.open("rb") | |||
| path = path.open("rb") # type: ignore[assignment] | |||
There was a problem hiding this comment.
Why not just use a new variable name here? I think it would be more readable and you would not need the type ignore.
There was a problem hiding this comment.
that would be indeed the normal way to solve it but because of the dispatch, it is not possible I think (or we would need to do multiple calls to open_path instead of one.
There was a problem hiding this comment.
Oh ok. It took me a while to get it and at first it looked like line 291 would lead to an infinite recursion in the "happy path". Only then I scrolled up and realized that the type assumptions are broken and this breaks the infinite recursion. I think it might have been easier to understand with two calls to to open_path. Or maybe even call open_path here and explicitly call open_path_pathlib below to make it even more clear that the below calls are "reattempts with better/corrected arguments.
There was a problem hiding this comment.
should be fixed in ee24538
| @@ -178,7 +178,9 @@ def json_to_data(json_string: str, custom_objects: Optional[Dict[str, Any]] = No | |||
|
|
|||
|
|
|||
| @functools.singledispatch | |||
| def open_path(path: Union[str, pathlib.Path, io.BufferedIOBase], mode: str, verbose: int = 0, suffix: Optional[str] = None): | |||
| def open_path( | |||
| path: Union[str, pathlib.Path, io.BufferedIOBase], mode: str, verbose: int = 0, suffix: Optional[str] = None | |||
There was a problem hiding this comment.
Why allow str paths when you reject them in the assert below?
There was a problem hiding this comment.
I need to check, might be because of the dispatch. (I didn't write that code)
There was a problem hiding this comment.
yes, because of the dispatch
| @@ -255,7 +260,7 @@ def open_path_pathlib(path: pathlib.Path, mode: str, verbose: int = 0, suffix: O | |||
|
|
|||
| if mode == "r": | |||
| try: | |||
| path = path.open("rb") | |||
| path = path.open("rb") # type: ignore[assignment] | |||
There was a problem hiding this comment.
Oh ok. It took me a while to get it and at first it looked like line 291 would lead to an infinite recursion in the "happy path". Only then I scrolled up and realized that the type assumptions are broken and this breaks the infinite recursion. I think it might have been easier to understand with two calls to to open_path. Or maybe even call open_path here and explicitly call open_path_pathlib below to make it even more clear that the below calls are "reattempts with better/corrected arguments.
Description
Motivation and Context
Types of changes
Checklist
make format(required)make check-codestyleandmake lint(required)make pytestandmake typeboth pass. (required)make doc(required)Note: You can run most of the checks using
make commit-checks.Note: we are using a maximum length of 127 characters per line