-
-
Notifications
You must be signed in to change notification settings - Fork 2.2k
Fix pkgutil.loader removal #3488
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
|
@KoolADE85 I had to make a bunch of changes to make the typing tests pass, can you take a final look? |
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.
Looks good. I made a few comments, but nothing stopping merge.
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.
Do you have a convention for using import typing vs. from typing import ...? If so, could you switch to that convention across all these type annotations?
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.
No conventions, some types are better import from typing_extensions to ensure backward compatibility in which case I prefer the from typing import ... syntax to easily switch between those.
| self.callback_map: dict = {} | ||
| # same deps as a list to catch duplicate outputs, and to send to the front end | ||
| self._callback_list = [] | ||
| self.callback_api_paths = {} | ||
| self._callback_list: list = [] | ||
| self.callback_api_paths: dict = {} |
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 dict assign the same type as Dict?
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.
Yes, we still support 3.8 but later versions of Python prefer using that with the list[int] syntax.
Recreation of #3476 with further fixes
Fix #3139