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

Add option to expand single star import #18

Merged
merged 1 commit into from Jun 16, 2017

Conversation

adhikasp
Copy link
Contributor

This patch add new feature to automatically expand a star
(wildcard) import to specify all names that used inside the code.

A sample code like this

from math import *
sin(1)
cos(0)

will be changed into

from math import cos, sin
sin(1)
cos(0)

Note that there are still 2 bugs regarding this feature,
which mainly caused by related upstream bug from pyflakes:

  1. A function/names that declared but later deleted by del
    command will raise a false positive that the names is
    undeclared and could possibly come from a star import
    (if present).
    False positive undefined name after del in branch pyflakes#175
  2. pyflakes is "inconsistent" on defining an undefined var
    in case of __all__ is used (like in module API files).
from foo import * # contain function_1 and function_2

__all__ = ['function_1', 'function_2', 'function_3']

function_2() # just use it somewhere to trigger pyflake

def function_3:
    return 'something'

pyflakes will complain that function_2 is undefined and
possibly come from module foo. The import then will be
expanded into...

from foo import function_2

But then pyflakes will complain function_1 is undefined
because it's used in __all__.

Closes #14

@coveralls
Copy link

coveralls commented Jun 15, 2017

Coverage Status

Coverage decreased (-98.7%) to 0.0% when pulling 0cf2a22 on adhikasp:expand-star-import into d756487 on myint:master.

@adhikasp
Copy link
Contributor Author

I will look into coverage decrease

@coveralls
Copy link

coveralls commented Jun 16, 2017

Coverage Status

Coverage decreased (-0.08%) to 98.583% when pulling 9a2ceec on adhikasp:expand-star-import into d756487 on myint:master.

Copy link
Member

@myint myint left a comment

Choose a reason for hiding this comment

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

Very nice!

And thanks for catching the missing parenthesis in the documentation. I didn't notice it for 4 years! 😄

autoflake.py Outdated
@@ -569,9 +607,11 @@ def _main(argv, standard_out, standard_error):
help='by default, only unused standard library '
'imports are removed; specify a comma-separated '
'list of additional modules/packages')
parser.add_argument('--expand-single-star-import', action='store_true',
Copy link
Member

@myint myint Jun 16, 2017

Choose a reason for hiding this comment

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

I'm not sure what the "single" means here. Do you think the shorter --expand-star-import would make sense?

Copy link
Member

@myint myint Jun 16, 2017

Choose a reason for hiding this comment

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

Also, I think it would better match the other options if it were plural (--expand-star-imports)

Copy link
Member

@myint myint Jun 16, 2017

Choose a reason for hiding this comment

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

Oh, never mind. I think I see what this means. It only works if there is a single star import in the whole file.

Copy link
Member

Choose a reason for hiding this comment

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

How about --expand-lone-star-imports?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah thats seems better

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, I think if we want to describe it in the option name, single is better from the meaning perpective http://wikidiff.com/lone/single 😄

autoflake.py Outdated
@@ -117,6 +117,20 @@ def unused_import_module_name(messages):
if module_name:
yield (message.lineno, module_name)

def star_import_used_line_numbers(messages):
"""Yield line number of star import usage"""
pattern = r':\ (.+)$'
Copy link
Member

@myint myint Jun 16, 2017

Choose a reason for hiding this comment

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

pattern doesn't seem to be used anywhere.

autoflake.py Outdated
for line_number, undefined_name, _ \
in star_import_usage_undefined_name(messages):
undefined_names.append(undefined_name)
if len(undefined_names) == 0:
Copy link
Member

Choose a reason for hiding this comment

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

It might be simpler to do:

if not undefined_names:

This patch add new feature to automatically expand a star 
(wildcard) import to specify all names that used inside the code.

A sample code like this

```python
from math import *
sin(1)
cos(0)
```

will be changed into 

```python
from math import cos, sin
sin(1)
cos(0)
```

Note that there are still 2 bugs regarding this feature, 
which mainly caused by related upstream bug from pyflakes:

1. A function/names that declared but later deleted by `del` 
   command will raise a false positive that the names is
   undeclared and could possibly come from a star import 
   (if present).
   PyCQA/pyflakes#175
2. pyflakes is "inconsistent" on defining an undefined var 
   in case of __all__ is used (like in module API files).
   
```python
from foo import * # contain function_1 and function_2

__all__ = ['function_1', 'function_2', 'function_3']

function_2() # just use it somewhere to trigger pyflake

def function_3:
    return 'something'
```

pyflakes will complain that function_2 is undefined and 
possibly come from module foo. The import then will be 
expanded into...

```python
from foo import function_2
```

But then pyflakes will complain function_1 is undefined 
because its used in `__all__`

Closes PyCQA#14
@adhikasp
Copy link
Contributor Author

All request has been amended

@coveralls
Copy link

coveralls commented Jun 16, 2017

Coverage Status

Coverage decreased (-0.09%) to 98.58% when pulling f5adfd3 on adhikasp:expand-star-import into d756487 on myint:master.

@myint myint merged commit ad7cc64 into PyCQA:master Jun 16, 2017
@myint
Copy link
Member

myint commented Jun 16, 2017

Thanks!

myint added a commit that referenced this pull request Jun 16, 2017
The Pyflakes bugs were mentioned in #18.
@myint
Copy link
Member

myint commented Jun 16, 2017

I put in a workaround (c07a90b) for the limitations mentioned in 1 and 2 above. It is conservative, but avoids those cases well enough to pass fuzz testing. We can remove them as Pyflakes improves.

@jayvdb
Copy link
Member

jayvdb commented Jun 17, 2017

Has a pyflakes bug been created about limitation "2" mentioned above?

@adhikasp
Copy link
Contributor Author

@jayvdb yes it is PyCQA/pyflakes#280

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.

None yet

4 participants