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

Fix: use __all__ to avoid noqa in import #70

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
5 participants
@skzinc
Contributor

skzinc commented Mar 21, 2018

closes #54
Used __all__ to avoid noqa in import

@yurishkuro

This comment has been minimized.

Member

yurishkuro commented Mar 21, 2018

is there some documentation about this being the convention in python?

@skzinc

This comment has been minimized.

Contributor

skzinc commented Mar 22, 2018

Hi Yuri,
I refered this to implement it. Since we are importing and using all of the sub-modules I think this would make code more cleaner. Also, there is an open issue which mentions the same.

@yurishkuro

@yurishkuro

This comment has been minimized.

Member

yurishkuro commented Mar 22, 2018

What happens if I make a typo when defining the __all__ variable, or if we rename one of the classes? With direct imports I'd expect the compiler to fail.

Are there unit tests that verify that all symbols can be correctly imported via from opentracing import X? If not, could you add them?

@carlosalberto

This comment has been minimized.

Contributor

carlosalberto commented Mar 22, 2018

Hey,

Using __all__ seems fine (just fetched the PR and tested locally - all fine and all tests work). But when it comes to lint, it fails (which was the reason behind having noqa in __init__.py):

nowhere:opentracing-python calberto$ make lint
flake8 opentracing tests
opentracing/__init__.py:23:1: F403 'from .span import *' used; unable to detect undefined names
opentracing/__init__.py:24:1: F403 'from .tracer import *' used; unable to detect undefined names
opentracing/__init__.py:25:1: F403 'from .propagation import *' used; unable to detect undefined names
opentracing/__init__.py:30:10: F405 Tracer may be undefined, or defined from star imports: .propagation, .span, .tracer
opentracing/propagation.py:101:80: E501 line too long (110 > 79 characters)
opentracing/tracer.py:228:80: E501 line too long (98 > 79 characters)
make: *** [lint] Error 1

I'm wondering if having the noqa in the files is really a problem (or else, if the errors above could be easily fixed by tunning __all__ ;) )

@SEJeff

This comment has been minimized.

SEJeff commented Apr 24, 2018

@yurishkuro __all__ is only used for from foo import * style imports, and limits the things being imported when that happens. Even when __all__ is implemented, you can still import specific things. In specific, it is specified how __all__ works in the python style guide pep8

Here is an example:

$ cat << EOF > foo.py
> __all__ = ('bar', 'barzz')
> 
> bar = "NOPE"
> 
> def barz():
>     pass
> EOF
$ python
Python 2.7.14 (default, Mar 14 2018, 13:36:31) 
[GCC 7.3.1 20180303 (Red Hat 7.3.1-5)] on linux2
Type "help", "copyright", "credits" or "license" for more information.

>>> from foo import *
Traceback (most recent call last):
  File "<console>", line 1, in <module>
AttributeError: 'module' object has no attribute 'barzz'
>>> from foo import bar
>>> from foo import barz
>>> barz
<function barz at 0x7f1ee2875320>
>>> bar
'NOPE'
$ python3.6
Python 3.6.5 (default, Apr  4 2018, 15:01:18) 
[GCC 7.3.1 20180303 (Red Hat 7.3.1-5)] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> from foo import *
Traceback (most recent call last):
  File "/usr/lib64/python3.6/code.py", line 91, in runcode
    exec(code, self.locals)
  File "<console>", line 1, in <module>
AttributeError: module 'foo' has no attribute 'barzz'
>>> from foo import bar, barz
>>> barz
<function barz at 0x7f066db370d0>
>>> bar
'NOPE'

I prefer using isort to automatically manage explicit imports, but this is zero harm in this specific change. It will be fully backwards compatible with anything that imports directly. My only nit with this PR is that __all__ can be a tuple, and that is generally more efficient (but this is a micro-optimization)

@zgfh

This comment has been minimized.

zgfh commented Apr 24, 2018

Is it better to use "# noinspection PyUnresolvedReferences" ?
see : https://stackoverflow.com/questions/21139329/false-unused-import-statement-in-pycharm

@SEJeff

This comment has been minimized.

SEJeff commented Apr 28, 2018

@zgfh that is only valid for developers using pycharm. I use vim (as an example) with some syntastic + flake8 on buffer save.

@carlosalberto

This comment has been minimized.

Contributor

carlosalberto commented May 10, 2018

I honestly think keeping the current way is just fine - on one hand, it let us be explicit and simple on what we expose, and we can also let the compiler report errors right away.

Moreover, from the docs:

Modules that are designed for use via from M import * should use the all mechanism to prevent exporting globals...

For opentracing we are not hiding anything, but simply explicitly exposing the members from a few submodules (in __init__.py), so we are good.

Finally, this doesn't really fix the lint situation regarding the # noqa.

@yurishkuro

This comment has been minimized.

Member

yurishkuro commented May 10, 2018

+1

Personally I would consider from foo import * to be a bad style anyway.

@carlosalberto

This comment has been minimized.

Contributor

carlosalberto commented Jul 11, 2018

Wondering if we should discard this PR, considering the feedback.
cc @yurishkuro

@SEJeff

This comment has been minimized.

SEJeff commented Jul 11, 2018

@skzinc skzinc closed this Jul 26, 2018

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