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 exported names starting with an underscore in __all__ #3746

Merged
merged 3 commits into from Jul 21, 2017

Conversation

Projects
None yet
2 participants
@daboross
Contributor

daboross commented Jul 20, 2017

Fixes #3745, if that's something we want to do.

This seemed like a fairly small change, so I went ahead and made this pull request. I'm not 100% sure the name module_public_even_with_underscore is best here, if there are any suggestions on this or any other part of the PR please mention them.

I added a new variable with the default False since it seemed like a good way to add this without introducing more overhead on any code which doesn't use this feature. I don't know this codebase very well though, so if there is a better or more clean way to do this, that would be excellent.

@ilevkivskyi

This comment has been minimized.

Show comment
Hide comment
@ilevkivskyi

ilevkivskyi Jul 20, 2017

Collaborator

Thanks for PR!

Concerning your question on gitter about from x import y as y in stubs, mypy currently does not follow this PEP 484 rule, there is PR #3706 to fix this. So that I would wait until that one is merged, and then consider its interaction with __all__.

Collaborator

ilevkivskyi commented Jul 20, 2017

Thanks for PR!

Concerning your question on gitter about from x import y as y in stubs, mypy currently does not follow this PEP 484 rule, there is PR #3706 to fix this. So that I would wait until that one is merged, and then consider its interaction with __all__.

@@ -1483,7 +1486,8 @@ def visit_import_all(self, i: ImportAll) -> None:
self.add_submodules_to_parent_modules(i_id, True)
for name, node in m.names.items():
node = self.normalize_type_alias(node, i)
if not name.startswith('_') and node.module_public:

This comment has been minimized.

@ilevkivskyi

ilevkivskyi Jul 20, 2017

Collaborator

Why isn't it possible to fix this by only changing the logic here? For example add a nested if that ignores the check for underscore here if m.names contains '__all__' TBH, I don't really like inclusion of a new flag.

@ilevkivskyi

ilevkivskyi Jul 20, 2017

Collaborator

Why isn't it possible to fix this by only changing the logic here? For example add a nested if that ignores the check for underscore here if m.names contains '__all__' TBH, I don't really like inclusion of a new flag.

This comment has been minimized.

@daboross

daboross Jul 20, 2017

Contributor

If we have access to __all__ here, not introducing a new flag does sound better! I wasn't sure of where we had access to what, so I was somewhat following how #1640 implemented __all__ originally.

Thank you for mentioning this, I'll change this.

@daboross

daboross Jul 20, 2017

Contributor

If we have access to __all__ here, not introducing a new flag does sound better! I wasn't sure of where we had access to what, so I was somewhat following how #1640 implemented __all__ originally.

Thank you for mentioning this, I'll change this.

This comment has been minimized.

@ilevkivskyi

ilevkivskyi Jul 20, 2017

Collaborator

Note that we don't actually need to access its content, we just need to know that it is there, since in this case all names not in __all__ will have module_public = False.

@ilevkivskyi

ilevkivskyi Jul 20, 2017

Collaborator

Note that we don't actually need to access its content, we just need to know that it is there, since in this case all names not in __all__ will have module_public = False.

This comment has been minimized.

@daboross

daboross Jul 20, 2017

Contributor

Ah right! Awesome, that's even better than what I misread you as saying - thanks!

@daboross

daboross Jul 20, 2017

Contributor

Ah right! Awesome, that's even better than what I misread you as saying - thanks!

@ilevkivskyi

Thanks! LGTM, I will merge this later if there are no objections.

@ilevkivskyi ilevkivskyi merged commit 2d67784 into python:master Jul 21, 2017

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@daboross

This comment has been minimized.

Show comment
Hide comment
@daboross

daboross Jul 21, 2017

Contributor

Thank you!

Contributor

daboross commented Jul 21, 2017

Thank you!

@daboross daboross deleted the daboross:support-exported-underscore-names branch Jul 21, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment