Skip to content

Support exported names starting with an underscore in __all__ #3746

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

Merged
merged 3 commits into from
Jul 21, 2017
Merged

Support exported names starting with an underscore in __all__ #3746

merged 3 commits into from
Jul 21, 2017

Conversation

daboross
Copy link
Contributor

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.

@@ -57,7 +57,8 @@ def is_similar_node_shallow(n: SymbolTableNode, m: SymbolTableNode) -> bool:
# type_override
if (n.kind != m.kind
or n.mod_id != m.mod_id
or n.module_public != m.module_public):
or n.module_public != m.module_public
or n.module_public_even_with_underscore != m.module_public_even_with_underscore):
Copy link
Contributor Author

@daboross daboross Jul 20, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wasn't sure of whether this should be included here or not, since it's affected by __all__ only. I would greatly appreciate if someone who has a better idea of the use of this function could comment on this.

@ilevkivskyi
Copy link
Member

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:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@daboross daboross Jul 20, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@daboross daboross Jul 20, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

@ilevkivskyi ilevkivskyi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@ilevkivskyi ilevkivskyi merged commit 2d67784 into python:master Jul 21, 2017
@daboross
Copy link
Contributor Author

Thank you!

@daboross daboross deleted the support-exported-underscore-names branch July 21, 2017 19:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants