Skip to content
This repository has been archived by the owner on May 23, 2023. It is now read-only.

Fix: use __all__ to avoid noqa in import #70

Closed
wants to merge 1 commit into from
Closed

Fix: use __all__ to avoid noqa in import #70

wants to merge 1 commit into from

Conversation

sudoshweta
Copy link
Contributor

closes #54
Used __all__ to avoid noqa in import

@yurishkuro
Copy link
Member

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

@sudoshweta
Copy link
Contributor Author

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

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
Copy link
Contributor

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

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

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

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
Copy link
Contributor

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

+1

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

@carlosalberto
Copy link
Contributor

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

@SEJeff
Copy link

SEJeff commented Jul 11, 2018

@carlosalberto +1

@sudoshweta sudoshweta closed this Jul 26, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

use __all__ to avoid noqa
5 participants