PyCQA / isort

A Python utility / library to sort imports.
https://pycqa.github.io/isort/
MIT License
6.45k stars 574 forks source link

Consider standardizing imports from the standard library #1914

Open NeilGirdhar opened 2 years ago

NeilGirdhar commented 2 years ago

I love isort and probably run it hundreds of times a day. It standardizes all of my imports so I never have to think about them.

One pain point I have is that I would like to standardize the imports from the standard library where there are aliases. Specifically, in Python 3.10, I believe that typing.Collection is equivalent to collections.abc.Collection. It would be nice to just pick one (ideally the latter, I think).

This may be a complicated feature since in earlier versions of Python, these are not equivalent. Probably the simplest implementation is for the user to present a list of pairs to isort for conversion.

ucodery commented 2 years ago

The typing module and abc module do have many similar classes, but they are not aliases. They can often be substituted in your own code, but isort should not be replacing objects in your code; especially same-named replacements.

>>> import collections.abc
>>> import typing
>>> collections.abc.Collection is typing.Collection
 False
>>> [same for same in dir(typing) if same in dir(collections.abc) and not same.startswith('__')] # none of these are the same
['AsyncGenerator',
 'AsyncIterable',
 'AsyncIterator',
 'Awaitable',
 'ByteString',
 'Callable',
 'Collection',
 'Container',
 'Coroutine',
 'Generator',
 'Hashable',
 'ItemsView',
 'Iterable',
 'Iterator',
 'KeysView',
 'Mapping',
 'MappingView',
 'MutableMapping',
 'MutableSequence',
 'MutableSet',
 'Reversible',
 'Sequence',
 'Set',
 'Sized',
 'ValuesView',
 '_CallableGenericAlias']

However, the standard library does have aliases. Prior to 3.10 Collection actually did have an alias, in collections. Notice that it's existence in on or the other or both is dependent on the python version.

>>> import collections
>>> import collections.abc
>>> collections.abc.Collection is collections.Collection
__main__:1: DeprecationWarning: Using or importing the ABCs from 'collections' instead of from 'collections.abc' is deprecated, and in 3.8 it will stop working
True

It could be nice for isort to standardize a code base to import one alias over another, but besides teaching isort what are aliases, how should it decide which one to prefer? For deprecations like above it might be obvious, but should from builtins import open or from io import open be preferred?

NeilGirdhar commented 2 years ago

The typing module and abc module do have many similar classes, but they are not aliases. T

Sorry, what I mean is that the versions from typing can be replaced with the versions from collections.abc at least in Python 3.10. This is true for all ordinary uses (uses as an annotation).

It could be nice for isort to standardize a code base to import one alias over another

I agree.

airallergy commented 2 years ago

+1 for this, quite a few of these types in typing have been deprecated since 3.9 as stated in the doc, after their counterparts in collections.abc/built-in started to support indexing.

It would be great if isort can support the standardisation, but I understand that the implementation can be quite tricky.

doublethefish commented 2 years ago

There are 3 issues here if I'm reading it correctly:

  1. Deprecated objects
  2. Duplicate names (called aliases here)
  3. Best practice issues

Whilst they are vaguely related to imports they are really "user errors" in domains other than import-order problems; this implies they are all linting issues and wouldn't be appropriate for isort to handle as it adheres to the single responsibility principle etc. I don't want/need isort duplicating work done by pylint or flake8 and other tools.

NeilGirdhar commented 2 years ago

There are 3 issues here if I'm reading it correctly: Deprecated objects Duplicate names (called aliases here) Best practice issues

Actually, just best practice issues of importing from collections over typing where possible.

it adheres to the single responsibility principle etc. I don't want/need isort duplicating work done by pylint or flake8 and other tools.

I agree with the principle of single responsibility. However, isort is in a unique position over the linting tools you mention since unlike the other tools, isort rewrites imports. This is why I requested the issue here instead of at Pylint or MyPy.

I concede that the principle makes sense from a purity point of view. So this is a request that isort broaden its goals as a more general import rewriter. And this issue provides some context for the kind of import rewriting that could be useful.

Another kind of import rewriting could be importing modules using

import jax.numpy as jnp  # Guarantees that jnp is a module.

over the more general symbol import

from jax import numpy as jnp  # jnp could be a symbol in jax.
doublethefish commented 2 years ago

Actually, just best practice issues of importing from collections over typing where possible.

"Actually," across the entire thread, the three issues are mentioned.

Another kind of import rewriting could be importing modules using

import jax.numpy as jnp # Guarantees that jnp is a module. over the more general symbol import

from jax import numpy as jnp # jnp could be a symbol in jax.

That's off-topic as it is a (confusing) separate issue:

  1. those aren't standard library imports (per issue title).
  2. it's a code-style issue not mentioned elsewhere in the thread.
NeilGirdhar commented 2 years ago

"Actually," across the entire thread, the three issues are mentioned.

I know, but as this is my issue, I'd like to restrict this issue to standardizing imports to the best ones.

That's off-topic as it is a (confusing) separate issue:

It's a response to your point about "single responsibility".

Personally, I use isort in my workflow as a tool with the single responsibilty of standardizing import lines. What isort currently does is:

However, I use autoflake to remove unused imports (3) as well.

And finally, there is no tool to standarize standard library import choices (4) nor standardizing module imports to the import ... as form (5).

These five tasks fall under the umbrella of standdizing import lines. I would like to see isort broadened to being a tool that standardizes all of the imports since:

Hopefully, this answers your concern about scope, but let me know if it doesn't and I'm happy to clarify.

ucodery commented 2 years ago

Whilst they are vaguely related to imports they are really "user errors" in domains other than import-order problems; this implies they are all linting issues and wouldn't be appropriate for isort to handle as it adheres to the single responsibility principle etc. I don't want/need isort duplicating work done by pylint or flake8 and other tools.

This is generally my view on it as well. I think isort should stick to sorting imports primarily. While it already does a few other tasks, I don't see that as permission to add in every formatting option anyone could want for imports into one tool. Standardizing import names, especially for types, would be a great new linter. Or maybe add it to pyupgrade as it's dealing with deprecated names.

I think my biggest pushback is that these lists would probably have to be manually curated per python minor version as the existence of new aliases or removal of old ones is rapidly changing with the growth of typing. isort is run as a static analysis tool and doesn't really know what version of python the code is written against. isort itself is often run under a different interpreter, at a different version, than the target project. It doesn't check that imports it writes back actually exist or can be resolved in the current environment, it just sorts them, sometimes with rewriting but always so that the same lookup path would be followed. It does have the --py option to specify a version of the standard library to target, but I'm not so sure it is used often and without that knowledge isort is left to guess - and python has some words about the temptation to guess. If this does go in I hope it is disabled by default unless turned on and/or --py is supplied.

Another kind of import rewriting could be importing modules using

The problem with this one is that import foo.bar will only work if bar is a submodule. And isort doesn't know what names are what types; it's actually incredibly difficult to say for sure what names are submodules without executing the real import.

isort works on files statically, one file at a time, and treats the imports mostly as text. These suggestions, besides adding creep to isort's responsibilities, would involve massive growth in the inspection engine of isort for no gain of the primary use of isort.

doublethefish commented 2 years ago

Personally, I use isort in my workflow as a tool with the single responsibilty of standardizing import lines

Gotcha 👍 The issue title and the example do not clearly reflect that.

These five tasks fall under the umbrella of standdizing import lines.

They do, but isort's main thing is to sort. Perhaps black, being a formatter, would be a better tool for standardising import lines. Maybe something based off of astroid.

I agree that a tool that could modify the import style would be useful, even if it was a dumb tool and broke my packages, there's value in increased readability for the places that do not break, and I would be willing to do the work to understand/fix the breaks - but that's probably of limited value overall because, as @ucodery points out, there are some issues with changes the import style and those can be very hard bugs to solve e.g. in tensor flow and other large packages you can't always be consistent with your import style and get the same results (python's blessing and its curse is its dynamicism).

NeilGirdhar commented 2 years ago

I don't see that as permission to add in every formatting option anyone could want for imports into one tool. Standardizing import names, especially for types, would be a great new linter. Or maybe add it to pyupgrade as it's dealing with deprecated names.

It's not a question of "permission" and I guess we disagree here. If we put the variety of standardization functions into different tools, then all that will happen is people will install all of these tools and run them in sequence using a script. Seems overcomplicated for users.

I think my biggest pushback is that these lists would probably have to be manually curated per python minor version

Yes, every Python might involve a few such definitions. I think that's a small development cost since Python's release cadence is 12 months.

isort is run as a static analysis tool and doesn't really know what version of python the code is written against

That's a good point, but it's easy enough to add a "target version range" option to the isort configuration.

It does have the --py option to specify a version of the standard library to target, but I'm not so sure it is used often and without that knowledge isort is left to guess -

No need to guess. If the target version range isn't explicitly specified, then no standardization needs to be done.

The problem with this one is that import foo.bar will only work if bar is a submodule

That's why I said that this should be done for submodules only. And that's why the comments I illustrated point out that this standardization provide an importer guarantee about the type of the imported symbol.

isort works on files statically, one file at a time, and treats the imports mostly as text. would involve massive growth in the inspection engine of isort for no gain of the primary use of isort.

First of all, the principle suggestion of this issue (standardizing the standard library imports) would not require any change to this textual interpretation or any introspection at all.

I understand your point about creep, but it has to be balanced against the value of the functionality from the user's perspective. Checking for unused imports for example, doesn't need too much in the way of introspection. It's not more than forty lines of code to iterate over the parse tree looking for names. And the value is enormous.

ucodery commented 2 years ago

It does have the --py option to specify a version of the standard library to target, but I'm not so sure it is used often and without that knowledge isort is left to guess -

No need to guess. If the target version range isn't explicitly specified, then no standardization needs to be done.

I think we agree there.

The problem with this one is that import foo.bar will only work if bar is a submodule

That's why I said that this should be done for submodules only. And that's why the comments I illustrated point out that this standardization provide an importer guarantee about the type of the imported symbol.

Yes, I think you understand the restrictions of when from foo import bar as baz can be changed to import foo.bar as baz. But my point is that I'm not sure that isort can understand when this is safe - not without a lot of new code - since it currently doesn't understand types or verify imports after writing them. But this is probably off-topic unless you are asking for both in this ticket.

isort works on files statically, one file at a time, and treats the imports mostly as text. would involve massive growth in the inspection engine of isort for no gain of the primary use of isort.

First of all, the principle suggestion of this issue (standardizing the standard library imports) would not require any change to this textual interpretation or any introspection at all.

I agree that the original ask would probably not require any new introspection, just some lookup tables.

I understand your point about creep, but it has to be balanced against the value of the functionality from the user's perspective. Checking for unused imports for example, doesn't need too much in the way of introspection. It's not more than forty lines of code to iterate over the parse tree looking for names. And the value is enormous.

Perhaps if there were no tools checking for unused imports now, it would be a great new feature. But there are already several other linting tools that check this, and adding it to isort will probably not have the effect of maintainers dropping those other tools as they check for other smells. In my view, if every tool had a strict single responsibility design, unused imports would be under the same tool as unused variable names and uncalled functions, not an import sorter. But are you actually adding this feature ask to the others already brought up?

NeilGirdhar commented 2 years ago

But my point is that I'm not sure that isort can understand when this is safe - not without a lot of new code

Yes, agreed.

But this is probably off-topic unless you are asking for both in this ticket.

Right, I'm not.

Perhaps if there were no tools checking for unused imports now, it would be a great new feature. But there are already several other linting tools that check this,

I'm not asking for a check though. I'm asking for import rewriting, which I currently do with

find . -name "*.py" ! -name "__init__.py" -exec autoflake --in-place --remove-all-unused-imports {} +

So, I disagree that there are "several linting tools that do this". There is one tool that does this, and it's not exactly accessible.

I agree that the original ask would probably not require any new introspection, just some lookup tables.

Great, we're on the same page.

But are you actually adding this feature ask to the others already brought up?

No, I only mentioned these in response to an argument about "single responsibility".