Skip to content
This repository has been archived by the owner on May 16, 2024. It is now read-only.

.isort.cfg is a conflicts bottleneck #625

Closed
guewen opened this issue Jan 10, 2020 · 15 comments · Fixed by #639
Closed

.isort.cfg is a conflicts bottleneck #625

guewen opened this issue Jan 10, 2020 · 15 comments · Fixed by #639

Comments

@guewen
Copy link
Member

guewen commented Jan 10, 2020

In 13.0 projects, when we run pre-commit and an addon uses a new python lib, seed-isort-config will add the lib in the file .isort.cfg to list it as "known third party". Example of resulting file:

[settings]
; see https://github.com/psf/black
multi_line_output=3
include_trailing_comma=True
force_grid_wrap=0
combine_as_imports=True
use_parentheses=True
line_length=88
known_odoo=odoo
known_odoo_addons=odoo.addons
sections=FUTURE,STDLIB,THIRDPARTY,ODOO,ODOO_ADDONS,FIRSTPARTY,LOCALFOLDER
known_third_party=setuptools

This file is a huge contender for conflicts when working with unmerged pull requests and, for instance, we use gitaggregator to merge them together.

A real life example: PR OCA/stock-logistics-warehouse#826 adds the cubiscan and mock libs, PR OCA/stock-logistics-warehouse#797 adds aiohttp. On aggregation, we'll have a conflict such as:

diff --cc .isort.cfg
index 969be420,6bcf3847..00000000
--- a/.isort.cfg
+++ b/.isort.cfg
@@@ -9,4 -9,4 +9,10 @@@ line_length=8
  known_odoo=odoo
  known_odoo_addons=odoo.addons
  sections=FUTURE,STDLIB,THIRDPARTY,ODOO,ODOO_ADDONS,FIRSTPARTY,LOCALFOLDER
++<<<<<<< HEAD
 +known_third_party=aiohttp,setuptools
++||||||| 5ded73cf
++known_third_party=setuptools
++=======
+ known_third_party=cubiscan,mock,setuptools
++>>>>>>> e7f58dcdc68ccd1d53389b1fe03ec94a796f830c

Of course, conflicts between pull requests are not new and are inevitable, but we generally try to avoid them. The situation with .isort.cfg is painful though.

Is ordering the imports so important? (I find the whole isort "infrastructure" complex for the added value)

Thoughts?
@pedrobaeza
Copy link
Member

I have also faced this and it's annoying, but not sure if there's a solution (except removing it).

@sbidoul you were the main creator of this, can you tell us?

@gurneyalex
Copy link
Member

Can we have this isort.cfg automatically merged with gitattributes, as in https://github.com/OCA/delivery-carrier/pull/243/files (this one is fore oca_dependencies.txt, but it could be generalized)

@sbidoul
Copy link
Member

sbidoul commented Jan 10, 2020

I was going to write about gitattributes too.

I wish isort would not need any configuration too.

@gurneyalex
Copy link
Member

OTOH I'm not sure gitattributes will work, since the conflict is on a single line...

@guewen
Copy link
Member Author

guewen commented Jan 10, 2020 •
edited
Loading

Can we have this isort.cfg automatically merged with gitattributes, as in https://github.com/OCA/delivery-carrier/pull/243/files (this one is fore oca_dependencies.txt, but it could be generalized)

I thought about this, but the union merge driver takes the whole lines from the 2 versions, so AFAIU we would have:

known_third_party=aiohttp,setuptools
known_third_party=cubiscan,mock,setuptools

Even if isort is able to parse this (not sure?), I don't think seed-isort-config can deal with it.

Git allows to use a custom script as merge driver, which would allow us to merge the actual content of the variable, but git does not allow to include this driver in .git/config (to prevent running arbitrary code on merges). The custom merge driver is then not a good solution.

@gurneyalex
Copy link
Member

could we leave out the isort config file, and have the seeding script run before the travis tests?

@guewen
Copy link
Member Author

guewen commented Jan 10, 2020 •
edited
Loading

I wish isort would not need any configuration too.

Especially since seed-isort-config overwrites the value with the libs it finds on every commit, so it'd no be required to keep the list in a file if isort was doing the same auto-detection of the libs on the fly. 😢

@sbidoul
Copy link
Member

sbidoul commented Jan 10, 2020

maybe you can setup your gitaggregate config with a merge strategy that ignores .isort.cfg

If not possible with git-aggregator as is, here was an attempt that might be useful after all: acsone/git-aggregator#20

@sbidoul
Copy link
Member

sbidoul commented Jan 10, 2020

The isort changelog has this sentence that might be interesting to investigate:

"Support for using requirements files to auto determine third-paty section if pipreqs & requirementslib are installed."

@guewen
Copy link
Member Author

guewen commented Jan 10, 2020

Great, I'll look into it. Another option is using https://github.com/asottile/reorder_python_imports instead of isort (to investigate)

guewen added a commit to guewen/stock-logistics-warehouse that referenced this issue Jan 13, 2020
@guewen
Add isort third party detection from requirements.txt
3e381b0 
The current setup used in the OCA for isort is done by using 2
pre-commit hooks:

1. seed-isort-config: find third-party libs and updates the
'.isort.cfg' file's "known_third_party" with the list
2. isort: runs isort, which will use the list of third party
libs provided by 'seed-isort-config'

This is an issue [0] with the workflow adopted by many contributors of
the OCA: as the pull requests may need some time to be merged, we use
temporary branches where we aggregate the various PRs.

If several pull requests add a third party library, we'll have a
conflict, and when the process of merging these pull requests is
automated (e.g. with git-aggregator [1]), it becomes tedious.

The list of libs in the "known_third_party" variable is not even
editable manually, as "seed-isort-config" overwrite the value on every
commit: it doesn't really make sense to actually store this.

As pointed out by @sbidoul [2], isort mentions in their changelog:

> Support for using requirements files to auto determine third-paty
> section if pipreqs & requirementslib are installed.

Which is the goal of this change, whose details are:

* Remove 'seed-isort-config' from pre-commit as the list of libs will be
  provided by requirements.txt
* Replace the isort pre-commit repo by the official repo (the mirror says
  it is been deprecated in favor of the official repo)
* Adds pipreqs and pip-api in additional dependencies to activate
  isort's feature to read requirements.txt [3]
* Add an union merge driver on 'requirements.txt' to resolve conflicts
on this file (on conflicts, both lines are kept, which works in most
cases on this file)

Alternatively, we could evaluate 'reorder-python-imports' in place of 'isort' [4]

I propose to evaluate this configuration on this repository, since this
is where I ran into the issue. If the result is good (my first test
shows it should), then we can apply the same commit on https://github.com/OCA/maintainer-quality-tools

[0] OCA/maintainer-quality-tools#625
[1] https://github.com/acsone/git-aggregator
[2] OCA/maintainer-quality-tools#625 (comment)
[3] https://github.com/timothycrosley/isort/blob/500bafabbd51a6005c11a00c4738a2438990e48a/pyproject.toml#L42
[4] https://github.com/asottile/reorder_python_imports
@guewen guewen mentioned this issue Jan 13, 2020
Closed
guewen added a commit to guewen/stock-logistics-warehouse that referenced this issue Jan 13, 2020
@guewen
Add isort third party detection from requirements.txt
e3c6187 
The current setup used in the OCA for isort is done by using 2
pre-commit hooks:

1. seed-isort-config: find third-party libs and updates the
'.isort.cfg' file's "known_third_party" with the list
2. isort: runs isort, which will use the list of third party
libs provided by 'seed-isort-config'

This is an issue [0] with the workflow adopted by many contributors of
the OCA: as the pull requests may need some time to be merged, we use
temporary branches where we aggregate the various PRs.

If several pull requests add a third party library, we'll have a
conflict, and when the process of merging these pull requests is
automated (e.g. with git-aggregator [1]), it becomes tedious.

The list of libs in the "known_third_party" variable is not even
editable manually, as "seed-isort-config" overwrite the value on every
commit: it doesn't really make sense to actually store this.

As pointed out by @sbidoul [2], isort mentions in their changelog:

> Support for using requirements files to auto determine third-paty
> section if pipreqs & requirementslib are installed.

Which is the goal of this change, whose details are:

* Remove 'seed-isort-config' from pre-commit as the list of libs will be
  provided by requirements.txt
* Replace the isort pre-commit repo by the official repo (the mirror says
  it is been deprecated in favor of the official repo)
* Adds pipreqs and pip-api in additional dependencies to activate
  isort's feature to read requirements.txt [3]
* Adds types: [python] in the pre-commit config: the mirror had it and
  the official repo doesn't. Without it, some files such as .pot are
  modified (different EOF).
* Add an union merge driver on 'requirements.txt' to resolve conflicts
on this file (on conflicts, both lines are kept, which works in most
cases on this file)

Alternatively, we could evaluate 'reorder-python-imports' in place of 'isort' [4]

I propose to evaluate this configuration on this repository, since this
is where I ran into the issue. If the result is good (my first test
shows it should), then we can apply the same commit on https://github.com/OCA/maintainer-quality-tools

[0] OCA/maintainer-quality-tools#625
[1] https://github.com/acsone/git-aggregator
[2] OCA/maintainer-quality-tools#625 (comment)
[3] https://github.com/timothycrosley/isort/blob/500bafabbd51a6005c11a00c4738a2438990e48a/pyproject.toml#L42
[4] https://github.com/asottile/reorder_python_imports
@guewen
Copy link
Member Author

guewen commented Jan 13, 2020

The isort changelog has this sentence that might be interesting to investigate:

"Support for using requirements files to auto determine third-paty section if pipreqs & requirementslib are installed."

I tried this here: OCA/stock-logistics-warehouse#828

It works well on the use case I described. After aggregating my pull requests and using only requirements.txt, isort does not modify any files 🎆

@sbidoul
Copy link
Member

sbidoul commented Jan 13, 2020

This makes requirements.txt more important where I was kind of hoping it could be removed at some point. Which makes me think we can generate it: OCA/oca-github-bot#98

guewen added a commit to guewen/maintainer-quality-tools that referenced this issue Jan 27, 2020
@guewen
Add isort third party detection from requirements.txt
79df308 
The current setup used in the OCA for isort is done by using 2
pre-commit hooks:

1. seed-isort-config: find third-party libs and updates the
'.isort.cfg' file's "known_third_party" option with the list
2. isort: runs isort, which will use the list of third party
libs provided by 'seed-isort-config' in the "known_third_party" option.

This is an issue [0] with the workflow adopted by many contributors of
the OCA: as the pull requests may need some time to be merged, we use
temporary branches where we aggregate the various PRs.

If several pull requests add a third party library, we'll have a
conflict, and when the process of merging these pull requests is
automated (e.g. with git-aggregator [1]), it becomes tedious.

The list of libs in the "known_third_party" variable is not even
editable manually, as "seed-isort-config" overwrite the value on every
commit: it doesn't really make sense to actually store this.

As pointed out by @sbidoul [2], isort mentions in their changelog:

> Support for using requirements files to auto determine third-paty
> section if pipreqs & requirementslib are installed.

Which is the goal of this change, details:

* Remove 'seed-isort-config' from pre-commit as the list of libs will be
  provided by requirements.txt
* Replace the isort pre-commit repo by the official repo (the mirror says
  it is been deprecated in favor of the official repo)
* Adds pipreqs and pip-api in additional dependencies to activate
  isort's feature to read requirements.txt [3]
* Adds types: [python] in the pre-commit config: the mirror had it and
  the official repo doesn't. Without it, some files such as .pot are
  modified (different EOF). (it can be removed after the next isort
  release)
* Add an union merge driver on 'requirements.txt' to resolve conflicts
  on this file (on conflicts, both lines are kept, which works in most
  cases on this file, in the rare case it's an issue because we have
  2 requirements for different versions, we'll have to fix manually)

Note: it is now important to have the Python libraries declared in
"requirements.txt" to have them ordered in the proper place.

Alternatively, we could have tried 'reorder-python-imports' in place of 'isort' [4].

This setup has been tested successfully on OCA/stock-logistics-warehouse#828

[0] OCA#625
[1] https://github.com/acsone/git-aggregator
[2] OCA#625 (comment)
[3] https://github.com/timothycrosley/isort/blob/500bafabbd51a6005c11a00c4738a2438990e48a/pyproject.toml#L42
[4] https://github.com/asottile/reorder_python_imports

Closes OCA#625
@guewen guewen mentioned this issue Jan 27, 2020
Closed
guewen added a commit to guewen/maintainer-quality-tools that referenced this issue Jan 27, 2020
@guewen
Add isort third party detection from requirements.txt
3f4d743 
The current setup used in the OCA for isort is done by using 2
pre-commit hooks:

1. seed-isort-config: find third-party libs and updates the
'.isort.cfg' file's "known_third_party" option with the list
2. isort: runs isort, which will use the list of third party
libs provided by 'seed-isort-config' in the "known_third_party" option.

This is an issue [0] with the workflow adopted by many contributors of
the OCA: as the pull requests may need some time to be merged, we use
temporary branches where we aggregate the various PRs.

If several pull requests add a third party library, we'll have a
conflict, and when the process of merging these pull requests is
automated (e.g. with git-aggregator [1]), it becomes tedious.

The list of libs in the "known_third_party" variable is not even
editable manually, as "seed-isort-config" overwrite the value on every
commit: it doesn't really make sense to actually store this.

As pointed out by @sbidoul [2], isort mentions in their changelog:

> Support for using requirements files to auto determine third-paty
> section if pipreqs & requirementslib are installed.

Which is the goal of this change, details:

* Remove 'seed-isort-config' from pre-commit as the list of libs will be
  provided by requirements.txt
* Replace the isort pre-commit repo by the official repo (the mirror says
  it is been deprecated in favor of the official repo)
* Adds pipreqs and pip-api in additional dependencies to activate
  isort's feature to read requirements.txt [3]
* Adds types: [python] in the pre-commit config: the mirror had it and
  the official repo doesn't. Without it, some files such as .pot are
  modified (different EOF). (it can be removed after the next isort
  release)
* Add an union merge driver on 'requirements.txt' to resolve conflicts
  on this file (on conflicts, both lines are kept, which works in most
  cases on this file, in the rare case it's an issue because we have
  2 requirements for different versions, we'll have to fix manually)

Note: it is now important to have the Python libraries declared in
"requirements.txt" to have them ordered in the proper place. At some
point, this file could be automatically generated: [4].

Alternatively, we could have tried 'reorder-python-imports' in place of 'isort' [5].

This setup has been tested successfully on OCA/stock-logistics-warehouse#828

[0] OCA#625
[1] https://github.com/acsone/git-aggregator
[2] OCA#625 (comment)
[3] https://github.com/timothycrosley/isort/blob/500bafabbd51a6005c11a00c4738a2438990e48a/pyproject.toml#L42
[4] OCA/oca-github-bot#98
[5] https://github.com/asottile/reorder_python_imports

Closes OCA#625
guewen added a commit to guewen/maintainer-quality-tools that referenced this issue Jan 31, 2020
@guewen
Add isort third party detection from requirements.txt
7c98c8b 
The current setup used in the OCA for isort is done by using 2
pre-commit hooks:

1. seed-isort-config: find third-party libs and updates the
'.isort.cfg' file's "known_third_party" option with the list
2. isort: runs isort, which will use the list of third party
libs provided by 'seed-isort-config' in the "known_third_party" option.

This is an issue [0] with the workflow adopted by many contributors of
the OCA: as the pull requests may need some time to be merged, we use
temporary branches where we aggregate the various PRs.

If several pull requests add a third party library, we'll have a
conflict, and when the process of merging these pull requests is
automated (e.g. with git-aggregator [1]), it becomes tedious.

The list of libs in the "known_third_party" variable is not even
editable manually, as "seed-isort-config" overwrite the value on every
commit: it doesn't really make sense to actually store this.

As pointed out by @sbidoul [2], isort mentions in their changelog:

> Support for using requirements files to auto determine third-paty
> section if pipreqs & requirementslib are installed.

Which is the goal of this change, details:

* Remove 'seed-isort-config' from pre-commit as the list of libs will be
  provided by requirements.txt
* Replace the isort pre-commit repo by the official repo (the mirror says
  it is been deprecated in favor of the official repo)
* Adds pipreqs and pip-api in additional dependencies to activate
  isort's feature to read requirements.txt [3]
* Adds types: [python] in the pre-commit config: the mirror had it and
  the official repo doesn't. Without it, some files such as .pot are
  modified (different EOF). (it can be removed after the next isort
  release)
* Add an union merge driver on 'requirements.txt' to resolve conflicts
  on this file (on conflicts, both lines are kept, which works in most
  cases on this file, in the rare case it's an issue because we have
  2 requirements for different versions, we'll have to fix manually)

Note: it is now important to have the Python libraries declared in
"requirements.txt" to have them ordered in the proper place. At some
point, this file could be automatically generated: [4].

Alternatively, we could have tried 'reorder-python-imports' in place of 'isort' [5].

This setup has been tested successfully on OCA/stock-logistics-warehouse#828

[0] OCA#625
[1] https://github.com/acsone/git-aggregator
[2] OCA#625 (comment)
[3] https://github.com/timothycrosley/isort/blob/500bafabbd51a6005c11a00c4738a2438990e48a/pyproject.toml#L42
[4] OCA/oca-github-bot#98
[5] https://github.com/asottile/reorder_python_imports

Closes OCA#625
@pedrobaeza
Copy link
Member

Any news about this?

@pedrobaeza
Copy link
Member

As a workaround, what about populating isort.cfg file with usual libraries in all repositories: setuptools, dateutil, etc.

I have needed to pre-add them in OCA/credit-control@b45b272 for avoiding conflicts in 4 simultaneous PRs to the same repo.

@sbidoul
Copy link
Member

sbidoul commented Feb 27, 2020

@pedrobaeza read from #629 (comment) for the latest on this topic. Someone has to try @yajo's proposal first.

sbidoul added a commit to acsone/maintainer-quality-tools that referenced this issue Mar 1, 2020
@sbidoul
sample files: simpler isort configuration
c8b0454 
fixes OCA#625
closes OCA#629
@sbidoul sbidoul mentioned this issue Mar 1, 2020
Merged
sbidoul added a commit to acsone/maintainer-quality-tools that referenced this issue Mar 2, 2020
@sbidoul
sample files: simpler isort configuration
5dd2828 
fixes OCA#625
closes OCA#629
@andrey-lepekhin andrey-lepekhin mentioned this issue Feb 14, 2023
Closed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
4 participants
@guewen @sbidoul @gurneyalex @pedrobaeza