[FEATURE] Support for `PEP 484` Redundant Module/Symbol Aliases about pycln HOT 18 CLOSED

@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?

from pycln.

@hadialqattan Please bump up the version~ 🎉 🥳

v2.0.1 has been released 🎉!

from pycln.

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

I'll get started on this right now 😃

from pycln.

@hadialqattan Please bump up the version~ 🎉 🥳

from pycln.

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.

from pycln.

@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?

from pycln.

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.

from pycln.

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

from pycln.

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.

from pycln.

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

from pycln.

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.

from pycln.

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?

from pycln.

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).

from pycln.

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.

from pycln.

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 ❤️.

from pycln.

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

from pycln.

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 🌚

from pycln.

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!

from pycln.