-
Notifications
You must be signed in to change notification settings - Fork 82
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
Conversation
57db1b6
to
0cf2a22
Compare
I will look into coverage decrease |
0cf2a22
to
9a2ceec
Compare
There was a problem hiding this 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', |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah thats seems better
There was a problem hiding this comment.
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':\ (.+)$' |
There was a problem hiding this comment.
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: |
There was a problem hiding this comment.
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
9a2ceec
to
f5adfd3
Compare
All request has been amended |
Thanks! |
The Pyflakes bugs were mentioned in #18.
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. |
Has a pyflakes bug been created about limitation "2" mentioned above? |
@jayvdb yes it is PyCQA/pyflakes#280 |
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
will be changed into
Note that there are still 2 bugs regarding this feature,
which mainly caused by related upstream bug from pyflakes:
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
in case of
__all__
is used (like in module API files).pyflakes will complain that function_2 is undefined and
possibly come from module foo. The import then will be
expanded into...
But then pyflakes will complain function_1 is undefined
because it's used in
__all__
.Closes #14