-
-
Notifications
You must be signed in to change notification settings - Fork 3k
Support importing types from nested packages #5591
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
Support importing types from nested packages #5591
Conversation
|
Is anyone available to look at this? |
|
@ethanhs Do you have time to look at this? This is related to PEP 561. |
|
Ah, sorry this PR got under my radar. I will take a look later this week. |
…nested-imports
5126318 to
9e5548f
Compare
|
Hi @ethanhs, were you able to have a look at this last week? |
|
I'll review this unless @ethanhs steals it back from me again. |
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.
I apologize for the long, nit-picking review! This is good work, but since the code you're changing is rather subtle I think it would be good to make the code as clear as possible. Please take my suggestions as attempts to help clarify both the existing code and the code you added. Thanks!
| module_not_found(manager, caller_line, caller_state, id) | ||
| raise ModuleNotFound | ||
| else: | ||
| elif root_source: |
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.
This could just be if since the previous if ḅlock always raises.
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.
Personally, I prefer the explicitness of using elif when chaining if statements but if its an issue I can change it.
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.
In that case it would behoove you to change the if on L2375 to elif as well. :-) I'll let it slide.
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.
@gvanrossum Thanks very much for your review! If there is anything I didn't satisfactorily address please let me know.
| module_not_found(manager, caller_line, caller_state, id) | ||
| raise ModuleNotFound | ||
| else: | ||
| elif root_source: |
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.
Personally, I prefer the explicitness of using elif when chaining if statements but if its an issue I can change it.
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.
I'll merge it now. Thanks for your efforts and for your attention to detail!
| module_not_found(manager, caller_line, caller_state, id) | ||
| raise ModuleNotFound | ||
| else: | ||
| elif root_source: |
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.
In that case it would behoove you to change the if on L2375 to elif as well. :-) I'll let it slide.
Attempts to address problems discussed in issue #1645. I enhanced the
mypy.build._find_moduleto search forpy.typedmarker files at all package levels instead of at the top level. This change currently doesn't update stub package loading but I have added a test that fails without my change and passes with it.