Skip to content
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

Define __all__ in gcloud/monitoring/__init__.py. #1747

Merged
merged 1 commit into from
Apr 25, 2016
Merged

Define __all__ in gcloud/monitoring/__init__.py. #1747

merged 1 commit into from
Apr 25, 2016

Conversation

rimey
Copy link
Contributor

@rimey rimey commented Apr 23, 2016

This is so that "from gcloud.monitoring import *" doesn't import
the package's module names.

This is so that "from gcloud.monitoring import *" doesn't import
the package's module names.
@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Apr 23, 2016
@rimey rimey added the api: monitoring Issues related to the Cloud Monitoring API. label Apr 23, 2016
@tseaver
Copy link
Contributor

tseaver commented Apr 23, 2016

-0, mostly because from foo import * is a truly evil pattern. I would definitely vote to reject any docs change which showed from gcloud.monitoring import *.

@tseaver
Copy link
Contributor

tseaver commented Apr 23, 2016

Looking at the module: there are no names present which are not listed in __all__.

@rimey
Copy link
Contributor Author

rimey commented Apr 23, 2016

This is a package, so it brings in all the module names: client, connection, label, metric, ...

@rimey
Copy link
Contributor Author

rimey commented Apr 23, 2016

from gcloud.monitoring import * is useful interactively.

@rimey
Copy link
Contributor Author

rimey commented Apr 23, 2016

>>> from gcloud import monitoring
>>> client = monitoring.Client('my-project')
>>> q = client.query(hours=2).align(Aligner.ALIGN_MEAN, minutes=5)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
NameError: name 'Aligner' is not defined
>>> from gcloud.monitoring import *
>>> q = client.query(hours=2).align(Aligner.ALIGN_MEAN, minutes=5)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
AttributeError: 'module' object has no attribute 'query'

@dhermes
Copy link
Contributor

dhermes commented Apr 23, 2016

I'm fine with defining __all__. But, @jonparrott recently pointed out that imported modules are not considered part of the public API. From PEP8

Imported names should always be considered an implementation detail. Other modules must not rely on indirect access to such imported names unless they are an explicitly documented part of the containing module's API, such as os.path or a package's init module that exposes functionality from submodules.

For an example from the stdlib.

>>> import subprocess
>>> subprocess.sys
<module 'sys' (built-in)>

@jgeewax
Copy link
Contributor

jgeewax commented Apr 23, 2016

I'm OK with this change -- though I'm -1 to using the import * pattern in our documentation.

If people want to use that interactively that's fine with me, but it's certainly not recommended.

@dhermes
Copy link
Contributor

dhermes commented Apr 23, 2016

Shall we merge then? @tseaver any objections?


Ditto on that. Namespaces are one of Python's strengths (both in language design and community / "idiomatic" behavior). From the Zen of Python:

Namespaces are one honking great idea -- let's do more of those!

@rimey
Copy link
Contributor Author

rimey commented Apr 23, 2016

Defining __all__ in this circumstance is standard practice. You just don't see it that much outside of carefully curated libraries like the Python standard library, because it's of minor consequence, for the reasons you have mentioned.

@tseaver
Copy link
Contributor

tseaver commented Apr 25, 2016

LGTM

@dhermes dhermes merged commit 8491da2 into googleapis:master Apr 25, 2016
@rimey rimey deleted the import-star branch April 25, 2016 17:35
@tseaver tseaver mentioned this pull request May 16, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: monitoring Issues related to the Cloud Monitoring API. cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants