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 Request] Optimize imports #333

Closed
devxpy opened this issue Jun 10, 2018 · 93 comments
Closed

[Feature Request] Optimize imports #333

devxpy opened this issue Jun 10, 2018 · 93 comments
Labels
help wanted Extra attention is needed T: enhancement New feature or request

Comments

@devxpy
Copy link

devxpy commented Jun 10, 2018

Can we please have a feature, similar to PyCharm's "optimize imports" feature, which basically sorts and removes un-used import statements?

P.S. Great work!, Black works like a charm for me.

@ambv ambv added T: enhancement New feature or request help wanted Extra attention is needed labels Jun 10, 2018
@ghost
Copy link

ghost commented Jun 13, 2018

Remove duplicate import also requested.

@hugovk
Copy link
Contributor

hugovk commented Jun 14, 2018

For the time being, you can use isort to sort imports and autoflake to remove unused imports.

isort -rc .
autoflake -r --in-place --remove-unused-variables .

@jensens
Copy link

jensens commented Jun 23, 2018

IMO import sorting should be handled separate outside of Black. With ISort (and others) we already have great tools.

@asottile
Copy link
Contributor

#363 (comment) also serves as a decent indicator that this won't / shouldn't happen in black.

from @ambv's comment:

Black is not a linter, it's a formatter. It starts and ends with the same AST

adding / removing / sorting imports would change the ast

@tweakimp
Copy link

tweakimp commented Jul 4, 2018

I wish there was something like black but for linting and code review. Maybe name it white? 😋

@denizdogan
Copy link

Obviously we can use isort and autoflake (or some other tools) to achieve the same thing, but a big benefit of Black is that it's opinionated and covers everything in one tool and one command and one optional configuration.

@ambv
Copy link
Collaborator

ambv commented Aug 17, 2018

I'm warming up to the idea that Black should cover this use case too.

@tweakimp
Copy link

Which one?
Sorting? Removing unused? Removing doubles?

@jaroel
Copy link

jaroel commented Aug 18, 2018

@ambv I understand why you'd want to, but pretty please stick to formatting only.
Not only should one tool do one thing (tm), we'd also be wasting the years of effort and tuning that went into tools like isort, pyflakes, pylint, et all - and at the integrations with IDEs - their options, switches and nuances.

Linting is a different kind of beast from formatting. I'm really comfortable with letting black format my code, as it doesn't change the working of it. But you have no idea why my code was written as it is.
For example: I need to have setup some paths before importing code - that's a given:

from configtype.jsonconfig import setup_search_paths
setup_search_paths(".")
from ocyan.main.settings import *  # isort:skip

I would expect that black-the-linter would always put all the imports together, without an cop-out:

from configtype.jsonconfig import setup_search_paths
from ocyan.main.settings import *
setup_search_paths(".")

I love that black takes out all discussion on formatting, but please please do not make black do linting (for lack of a better word).

Thank you for reading :)

@JelleZijlstra
Copy link
Collaborator

I would like to see Black auto-sort imports, actually. For my team the value of Black is in taking care of boring, unimportant code-related decisions in a consistent way, so that developers don't have to worry about these things. That value gets even bigger if we can have a single tool taking care of all of these decisions, instead of having one for formatting, one for sorting imports, etc. Otherwise, developers have to remember to run every one of those tools on their code, and it gets harder to get people to install editor plugins to run every tool automatically.

It's true that import sorting is a bit trickier than what Black currently does, because it will change the AST. To @jaroel's concern, we should limit import sorting to contiguous blocks of imports: if there is any non-import code, we should not move imports around it. Similarly, we should be careful not to reorder imports that define the same name, including import *s. We should also study how import sorting is done by existing tools like isort and learn from them.

@asottile
Copy link
Contributor

Otherwise, developers have to remember to run every one of those tools on their code

fwiw, pre-commit goes a long way towards attempting to solve this problem. It makes it easy to set up as many linters as you want (in a variety of languages) and runs them automatically (commit / push time and/or through CI).

I myself am leaning towards @jaroel's sentiment. black should aim to do formatting well and leave import sorting to the battle tested import sorting tools. Reinventing isort doesn't seem productive

@jaroel
Copy link

jaroel commented Aug 19, 2018

@JelleZijlstra It looks to me that you want automation, not import-sorting-by-black per se :)
The usual approach would be to wrap tools in a utility script, or use a Makefile to call everything in the correct order - or using a pre-commit hook.
ie, make lint calls isort and then black. This makes it even easier for your developer, as they won't even need to know about black anymore ;)

In other words: I wouldn't want my car mechanic to clean my clothes, even if it would be handy to have that if I'm already waiting at the shop ;)

@zsol
Copy link
Collaborator

zsol commented Aug 19, 2018

One practical problem we're running into for large codebases is that isort and black are slightly incompatible with each other, so running them after each other changes each file in the repository twice. Ideally, isort would have a black-compatible mode (or vice versa, but we have some reservations about isort's choice of style), but it looks like the maintainers of isort were not very responsive to PyCQA/isort#694

@ambv
Copy link
Collaborator

ambv commented Aug 19, 2018

@jaroel, you raise some valid points. The very reason I'm considering sorting imports in Black is that isort does it wrong:

  • In general, its implementation is problematic. Everything interesting happens in an __init__() method; it's based on string matching and not a syntax tree, and so on.
  • Its configuration based on magic discovery of first-party/third-party code only works on activated virtualenvs, making it brittle for CI.
  • It moves comments around in a way that is incompatible with Black.
  • However, the killer thing for me is the example you give around imports placed after function calls being moved above the calls. Black would never do that because there are simply too many cases where this breaks the file. This makes codemodding any larger codebase unsafe.

Even if isort's author agreed with some of the above, backwards compatibility concerns pretty much force him to keep it as is.

If I followed some of the advice here around "reinventing" things, Black wouldn't see the light of day at all. We've had autopep8 and YAPF the entire time, didn't we? Just as I didn't want to write an auto-formatter, I don't want to creep more scope into it. Sadly, sorting imports looks increasingly inevitable.

Don't worry though. This feature requires careful planning so it won't appear out of the blue any time soon.

@asottile
Copy link
Contributor

If I followed some of the advice here around "reinventing" things, Black wouldn't see the light of day at all. We've had autopep8 and YAPF the entire time, didn't we?

yapf/autopep8 have much more timid goals than black, not sure it's fair to compare them :) (whereas an import sorter would hopefully have identical goals).

That said, I also reinvented the isort wheel for exactly the reasons listed above (1 2 4) 🙃 (whoops I'm a hypocrite)

I definitely think (1) is fixable upstream. I've proposed fixes to (2) but this seems to be a sticking point (worked around instead), and (4) I just don't have any good excuses for isort -- this behaviour is unacceptable.

@digitalresistor
Copy link

@ambv is there anything we can do to help move this part of black along? Having fallen into various pitfalls of isort due to an issue of it's configuration (it merges .isort.cfg with ~/.isort.cfg for example, meaning a projects .isort.cfg has to define ALL keys or a users ~/.isort.cfg may override it), I'd love to see this implemented in black.

@JelleZijlstra
Copy link
Collaborator

JelleZijlstra commented Nov 28, 2018

@bertjwregeer I think the main limiting factor is that somebody has to actually do the work. Here's a list of some things that have to be taken care of:

  • Generally review what other current tools (isort, and I'm sure there are others) do. What kind of things to they change? What style do they choose? How to they decide what is safe to change?
  • Decide what the style is going to be that Black will enforce. Some questions to answer: Will we separate imports into different "sections", like internal vs. stdlib vs. third-party? How will we decide which imports go into which section? (I wouldn't want Black to go spelunking around the file system to figure out what is a third-party package, like isort apparently does.) Will we turn from x import y; from x import z into from x import y, z? (I would be OK with that but maybe it's a bridge too far.)
  • Figure out what the safety features are that we need. @ambv and I mentioned a few above, like not moving imports around comments or function bodies, and not moving things around import *, but do we need more?
  • We need to actually implement this in Black itself. We'll have to add a flag to skip import normalization. Import sorting will break Black's current safety check (which checks that the AST matches before and after), so we'll have to decide what to do about it (disable it or make it smarter).

@jaroel
Copy link

jaroel commented Nov 29, 2018

I've got a Zope/Grok project where we import the whole module and refer to that.
ie import z3c.form.widgets + z3c.form.widgets.select instead of from z3c.form.widgets import select.
It would be nice if that could be supported in some way?

@jensens
Copy link

jensens commented Nov 29, 2018

In the @plone project we decided to sort imports alphabetical. Major reason is, if there are many imports it is easier to read and to find an import, without guessing what it is semantically. An we often have many imports. Also, the internal vs. third-party is difficult to define in a project with many packages. Is the application server code third party or not? Developing an Addon for Plone, is Plone then thirdparty? If it is taken into Plone-Core, do we then need to resort imports? There is no semantic true way to make sections nor is there a good way to automate that.

Alphabetical imports are simple, easy to read and also do not need code inspection for sorting.

It still breaks the AST diff. I usually do an isort and the run black.

The Black compatible (and currently still on a line length of 79) Isort configuration.

@hugovk
Copy link
Contributor

hugovk commented Nov 29, 2018

@asottile's https://github.com/asottile/reorder_python_imports "has a single aim: reduce merge conflicts" and favours:

from typing import Dict
from typing import List

over:

from typing import Dict, List

@digitalresistor
Copy link

I much prefer grouping things together than a single import per line, I find the explosion of import lines to be distracting... but that all comes down to preference.

@tweakimp
Copy link

I think you can call it more than preference because if there is no other difference, less lines is better.
in hugovks example the second version is shorter, easier to read and should be preferred in my opinion.

@asottile
Copy link
Contributor

I have no stake in black's implementation, that said it's not just a stylistic preference. -- much like the rest of black there's a reason for its opinionated decisions. Many of them are similar to the rationale for the tool I've written: avoid merge conflicts, reduce diff churn.

The style chosen by reorder-python-imports has a single aim: reduce merge
conflicts.

By having a single import per line, multiple contributors can
add / remove imports from a single module without resulting in a conflict.

Consider the following example which causes a merge conflict:

# developer 1
-from typing import Dict, List
+from typing import Any, Dict, List
# developer 2
-from typing import Dict, List
+from typing import Dict, List, Tuple

no conflict with the style enforced by reorder-python-imports:

+from typing import Any
 from typing import Dict
 from typing import List
+from typing import Tuple

@gwax
Copy link

gwax commented Jan 7, 2019

Worth noting that black guarantees that AST of input matches AST of output. Reordering imports would break this guarantee.

Also, black plays along quite well with isort, so I'm not certain this is necessary.

@digitalresistor
Copy link

@gwax I would like to point you to: #333 (comment) where some issues with isort are laid out...

@ghost
Copy link

ghost commented Jan 8, 2019

Can we just get an option to disable checking imports? Otherwise my configured isort and black keep fighting with each other over which way is correct.

@abitrolly
Copy link

isort lacks uncompromising defaults, so projects like https://github.com/pypa/warehouse have to use a config.

[tool.isort]
profile = 'black'
lines_between_types = 1
combine_as_imports = true

@zsol
Copy link
Collaborator

zsol commented Sep 20, 2021

FWIW at my company we use usort together with Black, and in our own open source repos we encourage ufmt which combines the two.

Note that usort has a slightly different style than isort in Black-compatibility mode, but at least it's minimally configurable and gets the job done.

@WhyNotHugo
Copy link
Contributor

@zsol Have you considered something equivalent to isort's force_single_line=true?

It dramatically reduces merge conflict issues, which kinda aligns to how black approaches a lot of formatting too.

@danielbraun89
Copy link

danielbraun89 commented Oct 3, 2021

Black absolutely should not sort or touch imports whatsoever. Many times import order and convention are manually crafted to allow specific behavior (monkey patching, for example) and should never get mangled by a formatter. As noted above this is directly changing the program AST. This issue should get closed, buried, and never to be talked about again!😃

@stuaxo
Copy link

stuaxo commented Oct 6, 2021

I've seen a couple of larger projects that use a style with brackets:

from typing import (
    Any, 
    Dict, 
    List
)

This is still probably a easier to break during a merge than the style mentioned above:

from typing import Any
from typing import Dict
from typing import List

@WhyNotHugo
Copy link
Contributor

@stuaxo This syntax is very noisy when you have a single import though:

from typing import (
    Any,
)

You can special case the above and keep it to one line, but then the diff when adding a second import gets very noisy:

-from typing import Any
+from typing import (
+    Any,
+    Dict,
+)

Merge conflicts can also get annoying with this type of diff.

@dhvcc
Copy link

dhvcc commented Oct 7, 2021

I've seen a couple of larger projects that use a style with brackets:

from typing import (
    Any, 
    Dict, 
    List
)

This is still probably a easier to break during a merge than the style mentioned above:

from typing import Any
from typing import Dict
from typing import List

An important note should be that perfomance is different for every approach

In [4]: timeit('''
   ...: from typing import Any
   ...: from typing import Dict
   ...: from typing import List
   ...: ''', number=1_000_000)
Out[4]: 1.094976361026056

In [5]: timeit('''
   ...: from typing import Any, Dict, List
   ...: ''', number=1_000_000)
Out[5]: 0.4201086120447144

In [6]: timeit('''
   ...: from typing import (
   ...:     Any,
   ...:     Dict,
   ...:     List
   ...: )
   ...: ''', number=1_000_000)
Out[6]: 0.4278764280024916

@stuaxo
Copy link

stuaxo commented Oct 7, 2021

I've seen a couple of larger projects that use a style with brackets:

from typing import (
    Any, 
    Dict, 
    List
)

This is still probably a easier to break during a merge than the style mentioned above:

from typing import Any
from typing import Dict
from typing import List

An important note should be that perfomance is different for every approach

In [4]: timeit('''
   ...: from typing import Any
   ...: from typing import Dict
   ...: from typing import List
   ...: ''', number=1_000_000)
Out[4]: 1.094976361026056

In [5]: timeit('''
   ...: from typing import Any, Dict, List
   ...: ''', number=1_000_000)
Out[5]: 0.4201086120447144

In [6]: timeit('''
   ...: from typing import (
   ...:     Any,
   ...:     Dict,
   ...:     List
   ...: )
   ...: ''', number=1_000_000)
Out[6]: 0.4278764280024916

The difference in performance is pretty big, maybe the faster cpython project is interested in @markshannon @gvanrossum

@Timmmm
Copy link

Timmmm commented Oct 28, 2021

Black shouldn't sort imports by default for the reasons stated many times. But it should definitely be an option. Using isort separately can result in formatting conflicts between Black and isort. To the point that I find myself implementing some kind of loop like this:

do {
  old_code = code
  code = isort(black(code))
} while (code != old_code)

That obviously sucks.

@WhyNotHugo
Copy link
Contributor

WhyNotHugo commented Oct 28, 2021 via email

@Jackenmen
Copy link
Contributor

Jackenmen commented Oct 28, 2021

Using isort separately can result in formatting conflicts between Black and isort.

To my knowledge, the isort maintainer would consider any conflict with black that arises when using black profile as a bug or something that should be improved. If you have some issues with conflicts, I suggest reporting it there ;)

@autoferrit
Copy link

For me, I run autoflake, isort, and black and if I run black last, it reorders some imports. For example, if I have these imports after running isort, and before running black:

# Third Party
from jsonpath_ng.ext import parse

# Library
from myapp import utils as u
from myapp.custom_exceptions import (
    ...
)
from myapp.json_factory import JsonFactory

but after running black I have this

# Third Party

# Library
from myapp import utils as u
from myapp.custom_exceptions import (
    ...
)
from myapp.json_factory import JsonFactory
from jsonpath_ng.ext import parse

The jsonpath_ng package is a 3rd party package. So black is moving it to here when I feel like black should not touch the imports at all. This is my config for both in pyproject.toml

[tool.black]
line-length = 80
target-version = ['py38']
include = '''
(
      \.pyi?$
    | \.py?$
    | ^/tests/
    | ^/src/
)
'''
exclude = '''
(
  /(
      \.eggs         # exclude a few common directories in the
    | \.git          # root of the project
    | \.hg
    | \.mypy_cache
    | \.tox
    | \.venv
    | _build
    | __pycache__
    | buck-out
    | build
    | dist
    | .*/migrations
    | \.github
    | ci
    | node_modules
    | static
    | staticfiles
  )/
  | \.html
  | \.js
  | \.css
  | \.scss
)
'''

[tool.isort]
import_heading_firstparty = "Library"
import_heading_future = "Futures"
import_heading_local = "Local"
import_heading_stdlib = "Standard Library"
import_heading_thirdparty = "Third Party"
indent = 4
known_first_party = "myapp"
lines_after_imports = 2
lines_between_types = 1
profile = "black"

If people feel we must sort imports (I am fairly against this because it is easy to use these tools automatically), then why are we bothering to try and find out how to fix imports, just detect if isort is installed and call that to run the imports. if it is not, then don't do anything.

we could always install black like pip install black[isort] or something too to bring in that requirement. Like others, we have places in our code where imports need to be placed in the middle of a file or after other lines, which also helps with circular imports. So even then, if it is added it should be disabled by default.

@JelleZijlstra
Copy link
Collaborator

Black doesn't sort imports. I don't know what tool is reordering your imports, but it's not black.

@autoferrit
Copy link

I know it shouldn't now, but it is. running it directly from the cli using that as the config, black is changing imports after running isort. I am working on filling out a possible bug request to try and find out why.

@rmax
Copy link

rmax commented Nov 23, 2021

I use this additional settings and have no problem with running black and isort

# black compatibility settings
multi_line_output = 3
include_trailing_comma = true
force_grid_wrap = 0
use_parentheses = true
ensure_newline_before_comments = true

@janosh
Copy link

janosh commented Nov 23, 2021

@rmax You might try just

[isort]
profile = "black"

See isort docs on black compatibility.

@mcarans
Copy link

mcarans commented Jun 12, 2022

I am running blackd from PyCharm via the BlackConnect plugin. It would be great if black handled the imports as well. There is no working equivalent for isort on PyCharm. In any case, running separate daemons for each seems like overkill.

@zsol
Copy link
Collaborator

zsol commented Jun 13, 2022

I'm going to close this issue as we don't have plans for introducing import sorting into Black. There are several existing tools that provide import sorting really well, like isort and usort. If you're interested in a single tool that combines both, there's ufmt.

Edit: Check out the usort configuration options if you want to finetune import ordering to please your linter like @stefaneidelloth mentions below.

@zsol zsol closed this as completed Jun 13, 2022
@zsol zsol closed this as not planned Won't fix, can't repro, duplicate, stale Jun 13, 2022
wuisawesome pushed a commit to ray-project/ray that referenced this issue Jun 13, 2022
It will be easier to develop if we could use a tool to organize / sort imports and not have to move them around by hand.

This PR shows how we could do this with isort (black doesn't quite do this per psf/black#333)

After this PR lands everyone will need to update their formatter to include isort if they don't have it already, i.e.

   pip install -r ./python/requirements_linters.txt 

All future file changes will go through isort and may introduce a slightly larger PR the first time as it will clean up the imports. 

The plan is to land this PR and also clean up the rest of the code in parallel by using this PR to format the codebase (so people won't get surprised by the formatter if the file hasn't been touched yet)

Co-authored-by: Clarence Ng <[email protected]>
@stefaneidelloth
Copy link

I tried ufmt but it does not resolve my issues with the import order found by pylint:

third party import ... should be placed before ...

@psf psf locked and limited conversation to collaborators Jul 10, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
help wanted Extra attention is needed T: enhancement New feature or request
Projects
None yet
Development

No branches or pull requests