-
Notifications
You must be signed in to change notification settings - Fork 48
Modernize type annotation to use builtins #883
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
willingc
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree. Thanks @Czaki
|
|
||
| ```python | ||
| def napari_get_reader(path: Union[str, List[str]]) -> Optional[ReaderFunction] | ||
| def napari_get_reader(path: Union[str, list[str]]) -> Optional[ReaderFunction] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does it make sense to change this? it's from the hook_spec, so that would be older napari and older python, right? So maybe we should leave them to match the original code?
I'm not sure anyone is still migrating anything, so it might be moot...
| ```python | ||
| def napari_get_writer( | ||
| path: str, layer_types: List[str] | ||
| path: str, layer_types: list[str] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same comment as above, maybe we should leave this since it's from an era when List was the correct type hint.
| def writer( | ||
| path: str, layer_data: List[Tuple[Any, Dict, str]] | ||
| ) -> List[str] | ||
| path: str, layer_data: list[tuple[Any, dict, str]] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same as above.
|
Not a blocker at all, but maybe we shouldn't change the hook_spec code? It's from a different era and was correct at the time. Someone migrating -- though I doubt anyone is doing that now? -- would encounter that code. |
|
@psobolewskiPhD If you are at all concerned, perhaps the best thing would be to add a note to the doc that this applies to Python 3.9 and later. If someone wants to use 3.8 or earlier, they would need the older typing format. |
|
Eh. |
References and relevant issues
Description
We should not suggest using
Listwhen all supported Python versions supportlist[].