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

Configuration of one-per-line import splitting? #204

Closed
richardARPANET opened this issue May 11, 2018 · 10 comments
Closed

Configuration of one-per-line import splitting? #204

richardARPANET opened this issue May 11, 2018 · 10 comments

Comments

@richardARPANET
Copy link

Currently if there is an import line which is too long:

from my_app.something.blah import remove_weekends, get_daily_shape, another_import

Imports will be updated to be one per line. Not everyone wants this as it takes up vertical screen space adds more scrolling.

from my_app.something.blah import (
    remove_weekends,
    get_daily_shape,
    another_import,
)

Can we get a configuration option to update imports to take advantage of available space on each line? For example:

from my_app.something.blah import (
    remove_weekends, get_daily_shape, another_import,
)
@ambv
Copy link
Collaborator

ambv commented May 12, 2018

The current behavior is designed with two things in mind:

  • you can can reliably use isort (when configured with multiline mode 3 and trailing commas)
  • you can use git blame to easily see who added an import.

Black isn't configurable so far and I would like to avoid it.

@gaborbernat
Copy link
Contributor

@ambv the problem with this which I find is that it's not consistent. I mean adding another import may totally reformat the imports (and make 1 line, 12). Also there's a workaround to this:

from my_app.something.blah import remove_weekends, get_daily_shape
from my_app.something.blah import another_import

Now this will not be wrapped. I think isort can be made to work without this artificial long read. The git blame is long, but at the end of day I personally prefer four long lines of import with occasional merge conflicts, over 30 lines of import and no conflicts. Black not being configurable I can see the value in it. My question is that if it's worth the price of tons and tons of import lines for a nice git blame or not?

@ambv
Copy link
Collaborator

ambv commented May 13, 2018

@gaborbernat, originally I also had the second step of moving the imports like this first:

from my_app.something.blah import (
  remove_weekends, get_daily_shape, ...
)

Only if that line didn't fit I would explode entirely. The problem with that was that it was incompatible with isort and I don't want to implement import sorting myself if I can help it. I tried contacting the author of isort but without luck. Since quite a few people reported the problem, I decided to just do exactly what "mode 3" does in isort so the two tools can co-exist.

@richardARPANET
Copy link
Author

With the latest version of isort this works fine.

From:

from fdlkjfdlfldskjffkdjfdlksjflkd import fdfdfdsf, fdlkfjdsl, fdsfdsfdsfkjehkfhefekfh, fdsfdsfsd, fdskjflds,fefhekfhekfhehf, fkdjfdslkfj, fkjehfkjehfkjehkjfhekfhek, fsjflj,lkfjdflkjsffdsdfdsf

To:

from fdlkjfdlfldskjffkdjfdlksjflkd import (
    fdfdfdsf, fdlkfjdsl, fdsfdsfdsfkjehkfhefekfh, fdsfdsfsd, fdskjflds,
    fefhekfhekfhehf, fkdjfdslkfj, fkjehfkjehfkjehkjfhekfhek, fsjflj,
    lkfjdflkjsffdsdfdsf
)

With the following .isort.cfg

[settings]
line_length=79
indent='    '
multi_line_output=5

I suggest black uses this as it's a superior output.

@ambv
Copy link
Collaborator

ambv commented May 28, 2018

It's not superior. git blame lines can't tell who added a specific import and it's inconsistent with how Black is splitting lines in any other situation.

Pro-tip: if you pipe Black's output to isort you can keep any import sorting option you want.

@richardARPANET
Copy link
Author

It actually is. For the reasons I stated clearly above. Don't worry however, I'll simply fork the project.

@ambv
Copy link
Collaborator

ambv commented May 29, 2018

Good luck!

@jleclanche
Copy link

@ambv I think whether it's superior output or not is kind of missing the point. isort is a popular tool and black should try to play better with it. Not necessarily introduce 10 different ways of handling import sorting/reordering, but giving the user an option to not touch import statements at all.

@rwhitt2049
Copy link

Hold the line @ambv! The less configuration the better.

@jleclanche
Copy link

@rwhitt2049 You need to step back and try to understand why "less configuration" is a good philosophy.

The goal to avoid bikeshedding is very noble. Utilities like pylint/tslint are horrible because they require tons of configuration to even work in the first place, and then have switches and toggles for what end up being absurd features.

I already told Łukasz in a separate channel how much I respect Black's resistance to configuration, but it's very important to keep in mind when it actually achieves something versus not. And tooling compatibility is a pretty important thing.

There's precedent in prettier accepting an extra configuration switch (parentheses around single lambda arguments) to play nicely with a default setting of tslint. Prettier has a very similar philosophy as Black, the less config the better.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants