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

STYLE Reduce merge conflicts with force_single_line or force_grid_wrap=1 #50274

Closed
MarcoGorelli opened this issue Dec 15, 2022 · 2 comments
Closed
Labels
Code Style Code style, linting, code_checks

Comments

@MarcoGorelli
Copy link
Member

MarcoGorelli commented Dec 15, 2022

About 2 years ago, we introduced force_grid_wrap to reduce merge conflicts due to imports #39780

This helped, but we do better in the case when there's a single import per group.

For example, in #49736 the diff was:

- from typing import Iterator
+ from typing import (
+     Generic,
+     Iterator,
+     TypeVar,
+ )

, which caused conflicts when backporting to 1.5.x, because 1.5.x didn't have the Iterator import

If the import style had instead been:

from typing import Generic
from typing import Iterator
from typing import TypeVar

then the PR above could have just added the Generic and TypeVar imports, without touching the Iterator import, and the backport wouldn't have had this conflict

As an aside, this would also remove nearly 5,000 lines of code


This is how it would look like: #50275


An alternative would be to have forced

from typing import (
    Iterator,
)

, see #51693

This makes imports a lot longer, but has the same effect of reducing conflicts

@MarcoGorelli MarcoGorelli added the Code Style Code style, linting, code_checks label Dec 15, 2022
@MarcoGorelli MarcoGorelli changed the title STYLE Can we please use force_single_line in isort? STYLE Reduce merge conflicts with force_single_line Dec 15, 2022
@MarcoGorelli MarcoGorelli changed the title STYLE Reduce merge conflicts with force_single_line STYLE Reduce merge conflicts with force_single_line or force_grid_wrap=1 Feb 28, 2023
@bashtage
Copy link
Contributor

Thanks for knocking up the alternative.

I suppose both are a bit terrible but also both acceptable. The one full import per line is very natural with a single import, while the always using multiline is natural, at least for black users, with multiple imports. Ultimately I think both are worth the effort to improve the merge situation, so let's just let a majority of interested parties express an opinion and go with that.

@MarcoGorelli
Copy link
Member Author

closing as in the end people weren't too keen on this

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Code Style Code style, linting, code_checks
Projects
None yet
2 participants