Add isort third party detection from requirements.txt #629

guewen · 2020-01-27T08:18:53Z

The current setup used in the OCA for isort is done by using 2
pre-commit hooks:

seed-isort-config: find third-party libs and updates the
'.isort.cfg' file's "known_third_party" option with the list
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] #625
[1] https://github.com/acsone/git-aggregator
[2] #625 (comment)
[3] https://github.com/timothycrosley/isort/blob/500bafabbd51a6005c11a00c4738a2438990e48a/pyproject.toml#L42
[4] https://github.com/asottile/reorder_python_imports

Closes #625

sample_files/pre-commit-13.0/.pre-commit-config.yaml

sample_files/pre-commit-13.0/requirements.txt

sample_files/pre-commit-13.0/.pre-commit-config.yaml

guewen · 2020-01-27T09:48:51Z

@yajo thanks for your review, I pushed a fixup with the updates.

sample_files/pre-commit-13.0/.pre-commit-config.yaml

@sbidoul

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 · 2020-01-31T06:17:39Z

Squashed

moylop260

👍

sbidoul · 2020-02-08T13:15:42Z

@guewen One issue I detected with this is that the usual Odoo dependencies (e.g. lxml, dateutil) are usually not declared in requirements.txt and are now moved to the local imports group by isort.

Ah well... nothing is simple.

yajo · 2020-02-10T09:14:56Z

Do you have an example?

sbidoul · 2020-02-16T17:35:38Z

For example, any addon that imports dateutil will have it misplaced by isort, if known_thirparty does not declare dateutil.

Putting dateutil in the manifest external deps does not help, because what lands in requirements.txt is python-dateutil (which is correct) and not dateutil, and isort therefore does not detect it as third party.

One way to mitigate this is to have the list of common dependencies in known_thirdparty (it could be the list that is also known by pylint-odoo).

That would not automate it fully, as when a third party package has an import name different than it's distribution name, it would need to be added manually to known_thirdparty.

I'm experimenting with that in OCA/mis-builder#260.

BTW, I also wrote a small tool to generate requirements.txt from the manifests in case that ends up being useful: acsone/setuptools-odoo#42

yajo · 2020-02-19T09:53:03Z

OK I have another idea.

If we know the ODOO and ODOO_ADDONS sections... then can't we assume all other modules are THIRDPARTY?

isort docs say:

default_section: The default section to place imports in, if their section can not be automatically determined. FIRSTPARTY, THIRDPARTY, etc.

So if we add to .isort.cfg:

default_section=THIRDPARTY

We should be able to use isort exactly the same way without needing to seed it. No conflicts, same behavior. Could this be our missing silver bullet?

This is a test implementation of the hypothesis explained in OCA/maintainer-quality-tools#629 (comment), which aims to avoid unnecessary and constant git conflicts in `.isort.cfg` file.

yajo · 2020-02-28T09:04:56Z

My hypothesis is confirmed, that's the path to go.

See a real world scenario: OCA/product-attribute#582

sbidoul · 2020-02-29T14:04:07Z

Yes! Nice find @yajo

pedrobaeza · 2020-02-29T17:00:51Z

So are we deploying this config in all repos together with the prettier-xml thing?

fixes OCA#625 closes OCA#629

sbidoul · 2020-03-01T09:47:55Z

So are we deploying this config in all repos together with the prettier-xml thing?

@pedrobaeza That is what I intend to do yes, when #632 and #636 are cleared.

pedrobaeza · 2020-03-01T10:36:02Z

OK, thanks. @yajo can you help in finishing both?

yajo · 2020-03-02T08:32:49Z

sample_files/pre-commit-13.0/.pre-commit-config.yaml

    hooks:
      - id: isort
        name: isort except __init__.py
        exclude: /__init__\.py$
+        # TODO, the next time we upgrade isort's rev, we should remove 'types'
+        types: [python]
+        additional_dependencies: ["pipreqs==0.4.10", "pip-api==0.0.13"]


This is possibly not needed now.

yajo · 2020-03-02T08:33:09Z

sample_files/pre-commit-13.0/.isort.cfg

@@ -9,4 +9,3 @@ line_length=88
 known_odoo=odoo
 known_odoo_addons=odoo.addons
 sections=FUTURE,STDLIB,THIRDPARTY,ODOO,ODOO_ADDONS,FIRSTPARTY,LOCALFOLDER
-known_third_party=


Missing the default section, see #629 (comment).

sbidoul · 2020-03-02T09:05:27Z

@yajo this one is superseded by #639, if @guewen agrees.

fixes OCA#625 closes OCA#629

guewen · 2020-03-02T10:04:14Z

Yes, thanks folks!

This is a test implementation of the hypothesis explained in OCA/maintainer-quality-tools#629 (comment), which aims to avoid unnecessary and constant git conflicts in `.isort.cfg` file.

kittiu · 2020-04-03T06:17:30Z

Hi, by looking around seem that now we use THIRDPARTY in stead of define own known_thirdparty, right?

We got travis error in this PR OCA/server-tools#1684 as it need openpyxl

Where can I add it? Thanks!

sbidoul · 2020-04-03T07:13:18Z

@kittiu the issue you are having in that PR is not related to this. .isort.cfg is only used to control sorting of imports in python files, not for installation. I provided a solution as a comment in your the PR.

guewen force-pushed the pre-commit-isort-from-reqs branch from 79df308 to 3f4d743 Compare January 27, 2020 08:20

yajo suggested changes Jan 27, 2020

View reviewed changes

yajo approved these changes Jan 27, 2020

View reviewed changes

yajo reviewed Jan 27, 2020

View reviewed changes

sample_files/pre-commit-13.0/.pre-commit-config.yaml Show resolved Hide resolved

pedrobaeza mentioned this pull request Jan 27, 2020

[MIG] base_user_role: Migration to 13.0 OCA/server-backend#46

Merged

guewen force-pushed the pre-commit-isort-from-reqs branch from dcad340 to 7c98c8b Compare January 31, 2020 06:17

sbidoul approved these changes Jan 31, 2020

View reviewed changes

OCA-git-bot added the approved label Jan 31, 2020

moylop260 approved these changes Feb 4, 2020

View reviewed changes

sbidoul mentioned this pull request Feb 27, 2020

.isort.cfg is a conflicts bottleneck #625

Closed

yajo mentioned this pull request Feb 28, 2020

[FIX] Remove isort seed OCA/product-attribute#582

Merged

sbidoul added a commit to acsone/maintainer-quality-tools that referenced this pull request Mar 1, 2020

sample files: simpler isort configuration

c8b0454

fixes OCA#625 closes OCA#629

sbidoul mentioned this pull request Mar 1, 2020

sample files: simpler isort configuration #639

Merged

yajo suggested changes Mar 2, 2020

View reviewed changes

OCA-git-bot removed the approved label Mar 2, 2020

sbidoul added a commit to acsone/maintainer-quality-tools that referenced this pull request Mar 2, 2020

sample files: simpler isort configuration

5dd2828

fixes OCA#625 closes OCA#629

guewen closed this Mar 2, 2020

sergio-teruel mentioned this pull request Mar 4, 2020

[FIX] Remove isort seed OCA/credit-control#57

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add isort third party detection from requirements.txt #629

Add isort third party detection from requirements.txt #629

guewen commented Jan 27, 2020

guewen commented Jan 27, 2020

guewen commented Jan 31, 2020

moylop260 left a comment

sbidoul commented Feb 8, 2020

yajo commented Feb 10, 2020

sbidoul commented Feb 16, 2020 •

edited

Loading

yajo commented Feb 19, 2020

yajo commented Feb 28, 2020

sbidoul commented Feb 29, 2020

pedrobaeza commented Feb 29, 2020

sbidoul commented Mar 1, 2020

pedrobaeza commented Mar 1, 2020

yajo Mar 2, 2020

yajo Mar 2, 2020

sbidoul commented Mar 2, 2020

guewen commented Mar 2, 2020

kittiu commented Apr 3, 2020

sbidoul commented Apr 3, 2020

Add isort third party detection from requirements.txt #629

Add isort third party detection from requirements.txt #629

Conversation

guewen commented Jan 27, 2020

guewen commented Jan 27, 2020

guewen commented Jan 31, 2020

moylop260 left a comment

Choose a reason for hiding this comment

sbidoul commented Feb 8, 2020

yajo commented Feb 10, 2020

sbidoul commented Feb 16, 2020 • edited Loading

yajo commented Feb 19, 2020

yajo commented Feb 28, 2020

sbidoul commented Feb 29, 2020

pedrobaeza commented Feb 29, 2020

sbidoul commented Mar 1, 2020

pedrobaeza commented Mar 1, 2020

yajo Mar 2, 2020

Choose a reason for hiding this comment

yajo Mar 2, 2020

Choose a reason for hiding this comment

sbidoul commented Mar 2, 2020

guewen commented Mar 2, 2020

kittiu commented Apr 3, 2020

sbidoul commented Apr 3, 2020

sbidoul commented Feb 16, 2020 •

edited

Loading