Skip to content

Support importing types from nested packages - #5591

Merged
gvanrossum merged 7 commits into
python:masterfrom
chrisphilip322:support-nested-imports
Sep 26, 2018
Merged

Support importing types from nested packages#5591
gvanrossum merged 7 commits into
python:masterfrom
chrisphilip322:support-nested-imports

Conversation

@chrisphilip322

@chrisphilip322chrisphilip322 commented Sep 9, 2018

Copy link
Copy Markdown
Contributor

Attempts to address problems discussed in issue #1645. I enhanced the mypy.build._find_module to search for py.typed marker 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.

@chrisphilip322

chrisphilip322 commented Sep 17, 2018

Copy link
Copy Markdown
ContributorAuthor

Is anyone available to look at this?

@JukkaL

JukkaL commented Sep 17, 2018

Copy link
Copy Markdown
Collaborator

@ethanhs Do you have time to look at this? This is related to PEP 561.

@emmatyping

emmatyping commented Sep 17, 2018

Copy link
Copy Markdown
Member

Ah, sorry this PR got under my radar. I will take a look later this week.

@chrisphilip322

chrisphilip322 commented Sep 24, 2018

Copy link
Copy Markdown
ContributorAuthor

Hi @ethanhs, were you able to have a look at this last week?

@gvanrossumgvanrossum self-assigned this Sep 24, 2018
@gvanrossum

gvanrossum commented Sep 24, 2018

Copy link
Copy Markdown
Member

I'll review this unless @ethanhs steals it back from me again.

gvanrossum
gvanrossum reviewed Sep 25, 2018
View reviewed changes

@gvanrossumgvanrossum left a comment

Copy link
Copy Markdown
Member

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!

Comment threadmypy/build.py Outdated
Comment threadmypy/build.py Outdated
Comment threadmypy/build.py Outdated
Comment threadmypy/build.py
Comment threadmypy/build.py
module_not_found(manager, caller_line, caller_state, id)
raise ModuleNotFound
else:
elif root_source:

@gvanrossumgvanrossumSep 24, 2018

Copy link
Copy Markdown
Member

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.

@chrisphilip322chrisphilip322Sep 25, 2018

Copy link
Copy Markdown
ContributorAuthor

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.

@gvanrossumgvanrossumSep 26, 2018

Copy link
Copy Markdown
Member

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.

Comment threadmypy/build.py Outdated
Comment threadmypy/build.py Outdated
Comment threadmypy/build.py
Comment threadmypy/test/testpep561.py Outdated
Comment threadtest-data/packages/typedpkg_namespace.alpha/setup.py
chrisphilip322
chrisphilip322 commented Sep 25, 2018
View reviewed changes

@chrisphilip322chrisphilip322 left a comment

Copy link
Copy Markdown
ContributorAuthor

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.

Comment threadmypy/build.py
Comment threadmypy/build.py
Comment threadmypy/build.py
module_not_found(manager, caller_line, caller_state, id)
raise ModuleNotFound
else:
elif root_source:

@chrisphilip322chrisphilip322Sep 25, 2018

Copy link
Copy Markdown
ContributorAuthor

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.

gvanrossum
gvanrossum approved these changes Sep 26, 2018
View reviewed changes

@gvanrossumgvanrossum left a comment

Copy link
Copy Markdown
Member

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!

Comment threadmypy/build.py
module_not_found(manager, caller_line, caller_state, id)
raise ModuleNotFound
else:
elif root_source:

@gvanrossumgvanrossumSep 26, 2018

Copy link
Copy Markdown
Member

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.

@gvanrossum
gvanrossum merged commit 2244721 into python:masterSep 26, 2018
@chrisphilip322chrisphilip322 mentioned this pull request Oct 9, 2018
Closed
@gvanrossumgvanrossum mentioned this pull request Oct 9, 2018
Closed
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@gvanrossumgvanrossumgvanrossum approved these changes

Assignees

@gvanrossumgvanrossum

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@chrisphilip322@JukkaL@emmatyping@gvanrossum