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

[FEATURE] Support for PEP 484 Redundant Module/Symbol Aliases #149

Closed
adam-grant-hendry opened this issue Jul 4, 2022 · 18 comments · Fixed by #150
Closed

[FEATURE] Support for PEP 484 Redundant Module/Symbol Aliases #149

adam-grant-hendry opened this issue Jul 4, 2022 · 18 comments · Fixed by #150
Labels
enhancement New feature or request feature New things

Comments

@adam-grant-hendry
Copy link

Is your feature request related to a problem? Please describe.

PEP 484 states

Modules and variables imported into the stub are not considered exported from the stub unless the import uses the import ... as ... form or the equivalent from ... import ... as ... form. (UPDATE: To clarify, the intention here is that only names imported using the form X as X will be exported, i.e. the name before and after as must be the same.)

Pyright calls

  • import A as A a redundant module alias
  • from X import A as A a redundant symbol alias

These imports are needed, but not actually used:

  • per PEP 484, to allow static type checkers to see the module's contents, and
  • for package maintainers, as a courtesy to users to avoid them having to use long dotted access lookups (Law of Demeter Violation)

These typically occur in __init__.py and __init__.pyi files, but I don't want to ignore these files entirely. I would also like to run pycln on them to remove old-form uppercase typing module types (e.g. List, Optional, Dict, etc.) after running pyupgrade and to find genuinely unused imports.

Currently, the pycln CLI arguments --exclude and --extend-exclude only accept files and path regex's, not regex's matching specific lines in modules.

To have pycln ignore these aliases, one currently must add a # nopycln: import to each import in the project that is being exported, which is quite tedious.

Describe the solution you'd like. A clear and concise description of what you want to happen.

Since PEP 484 redundant aliases have a specific form, it would be nice to have a regex feature to exclude imports of a specific form; i.e. those matching the patterns

  • import A as A and
  • from X import A as A

A backreference could be used in a regular expression, like so:

/import (\w*) as \1/gm

and

/from (\w*) import (\w*) as (\2)/gm

respectively.

Since the module/symbol name is being repeated (A) instead of renamed, it is clear these are intended for the purpose of exporting (as opposed to, say, import numpy as np or from matplotlib import pyplot as plt), so there is minimal risk of accidentally skipping an actually unused import.

Describe alternatives you've considered A clear and concise description of any alternative solutions or features you've considered.
N/A

Additional context Add any other context or screenshots about the feature request here.
N/A

@adam-grant-hendry adam-grant-hendry added enhancement New feature or request feature New things labels Jul 4, 2022
@hadialqattan
Copy link
Owner

Hi @adam-grant-hendry, first of all, I'm happy to see you around and supporting the project. Second, this feature request is accepted and will be added in the next release (perhaps today). Third, thanks for providing a regex, but Pycln is based on code AST analysis instead of regex, so now I'm trying to find a way to parse the AST of .pyi files that supports all the supported Python versions by Pycln.

@adam-grant-hendry
Copy link
Author

@hadialqattan Thank you for your support!

Second, this feature request is accepted and will be added in the next release (perhaps today).

That's wonderful! It would be great if it could be added today! Congratulations! 🎉

Third, thanks for providing a regex, but Pycln is based on code AST analysis instead of regex, so now I'm trying to find a way to parse the AST of .pyi files that supports all the supported Python versions by Pycln.

No problem at all. A regex was simply my first thought. I would be happy, personally, however it is implemented!

Thank you and your team for this wonderful package!

Question: How do you intend the feature will work? Will it be automatic, or will the user have to set a certain flag on the command line?

@hadialqattan
Copy link
Owner

hadialqattan commented Jul 4, 2022

Question: How do you intend the feature will work? Will it be automatic, or will the user have to set a certain flag on the command line?

I think this should be automatic just like what pycln does with .py files, what do you think?

@hadialqattan
Copy link
Owner

BTW, I'm not really familiar with .pyi files, but as far as I know (by reading PEP484) there are no edge cases that are true for .pyi and not for .py files (regarding importing).

@adam-grant-hendry
Copy link
Author

I think this should be automatic just like what pycln does with .py files, what do you think?

BTW, I'm not really familiar with .pyi files, but as far as I know (by reading PEP484) there are no edge cases that are true for .pyi and not for .py files (regarding importing).

That should be true, yes. AFAIK, the redundant nature (A as A) was intentional in PEP 484 for this reason: 99.99% of imports that are doing an import as are usually trying to rename the import (e.g. import numpy as np). Historically, most linters (flake8, pylint, etc.) would flag an import A as A as a kind of "useless-import" or "does not rename the import"; something to that effect. Those warnings have to be silenced for specific files in that instance (or specific lines with # noqa or # pylint: disable=)

I believe it is safe to say that the number of redundant imports (A as A) used purposefully for exporting would far outweigh any edge cases. Specifically, because these imports are now required by the PEP, so authors are having to refactor their existing __init__.py files (and others where this happens).

For any possible edge cases, you might need an argument like

--remove-imports

that always removes a named import regardless.

As a bonus, such an argument might be double-useful for authors who wish to purposefully remove a used import everywhere it appears where they want to force contributors to use a specific package.

However, honestly, I personally think the author could manually remove the import in that instance and adding an extra CLI argument like --remove-imports isn't necessary.

@adam-grant-hendry
Copy link
Author

As an alternative, for backwards compatibility, you could optionally add a CLI argument

--remove-redundant-aliases

so users can opt-in or opt-out of the feature.

@adam-grant-hendry
Copy link
Author

adam-grant-hendry commented Jul 4, 2022

Lastly, I think it is VERY IMPORTANT to add a small bit of documentation explaining the new behavior.

PEP 484 is still not known by most people, so they might get confused that something their linter (flake8/pylint/etc.) previously flagged as "useless" or "not renaming anything" was kept by pycln.

It could be as simple as saying

pycln keeps redundant alias imports:

import A as A

and redundant alias symbols

from A import B as B

in compliance with PEP 484 for the purposes of exporting modules and symbols for static type checking. See PEP 484 Stub Files for details

@hadialqattan
Copy link
Owner

As an alternative, for backwards compatibility, you could optionally add a CLI argument

--remove-redundant-aliases

so users can opt-in or opt-out of the feature.

I see your point, but I think that PEP484 is old enough to be the default behavior without an opt-out arg. Moreover, this feature is totally safe because it's about skipping certain types of imports (not removing them) which can't break users' programs.

@hadialqattan
Copy link
Owner

Also, for simplicity, I believe that CLI tools should be default-first instead of having too many CLI arg that users should use.

@hadialqattan
Copy link
Owner

Lastly, I think it is VERY IMPORTANT to add a small bit of documentation explaining the new behavior.

PEP 484 is still not known by most people, so they might get confused that something their linter (flake8/pylint/etc.) previously flagged as "useless" or "not renaming anything" was kept by pycln.

It could be as simple as saying

pycln keeps redundant alias imports:
import A as A
and redundant alias symbols
from A import B as B
in compliance with PEP 484 for the purposes of exporting modules and symbols for static type checking. See PEP 484 Stub Files for details

For sure. Thanks a lot for the suggested note ❤️.

@hadialqattan
Copy link
Owner

@adam-grant-hendry if you don't mind, could you test the master branch before creating a new release?

@adam-grant-hendry
Copy link
Author

@hadialqattan Completely agree with you on all points. Thank you so much for this!

I can test early tomorrow as I am spending time with family right now for the 4th of July.

To clarify, would you like me to write unit tests or just confirm it works on my machine?

@hadialqattan
Copy link
Owner

I can test early tomorrow as I am spending time with family right now for the 4th of July.

Happy Treason Day!

To clarify, would you like me to write unit tests or just confirm it works on my machine?

Only confirming it works on your machine the way you meant it to be.

@adam-grant-hendry
Copy link
Author

Only confirming it works on your machine the way you meant it to be.

I'll get started on this right now 😃

@hadialqattan
Copy link
Owner

hadialqattan commented Jul 5, 2022

Only confirming it works on your machine the way you meant it to be.

I'll get started on this right now 😃

Bro plz don't bother yourself thinking of every possible edge case, I mean you could test it only against the file(s) that you wanted to cleanup so you opened this issue 🌚

@adam-grant-hendry
Copy link
Author

Bro plz don't bother yourself thinking of every possible edge case, I mean you could test it only against the file(s) that you wanted to cleanup so you opened this issue 🌚

😆 Thank bro! I ran the master branch on my files and confirm it's working as I expected!

@adam-grant-hendry
Copy link
Author

@hadialqattan Please bump up the version~ 🎉 🥳

@hadialqattan
Copy link
Owner

@hadialqattan Please bump up the version~ 🎉 🥳

v2.0.1 has been released 🎉!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request feature New things
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants