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.

Sign up for GitHub

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

Jump to bottom

[13.0] Add isort third party detection from requirements.txt #828

Closed
wants to merge 2 commits into from
Closed

[13.0] Add isort third party detection from requirements.txt #828

wants to merge 2 commits into from

Conversation

guewen
Copy link
Member

@guewen guewen commented Jan 13, 2020 •
edited
Loading

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 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), 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, 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
  • 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'

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

@guewen guewen requested a review from sbidoul January 13, 2020 08:35
@guewen guewen changed the title Add isort third party detection from requirements.txt [13.0] Add isort third party detection from requirements.txt 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 guewen force-pushed the 13.0-pre-commit-isort-reqs branch from 3e381b0 to e3c6187 Compare January 13, 2020 08:59
@guewen guewen mentioned this pull request Jan 13, 2020
Closed
sbidoul
sbidoul approved these changes Jan 13, 2020
View reviewed changes
Copy link
Member

@sbidoul sbidoul left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @guewen, thanks for the detailed writeup. This looks like a great improvement.

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

I like the separate import groups we do for odoo and odoo.addons, but if isort becomes too much of a trouble, it's certainly not a blocking point.

@guewen guewen mentioned this pull request Jan 24, 2020
Merged
guewen added a commit to guewen/maintainer-quality-tools that referenced this pull request 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 pull request Jan 27, 2020
Closed
guewen added a commit to guewen/maintainer-quality-tools that referenced this pull request 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
yajo
yajo suggested changes Jan 27, 2020
View reviewed changes
guewen added a commit to guewen/maintainer-quality-tools that referenced this pull request 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
@guewen
Copy link
Member Author

guewen commented Mar 3, 2020

Closing, to replace by OCA/maintainer-quality-tools#639

@guewen guewen closed this Mar 3, 2020
@guewen
Copy link
Member Author

guewen commented Mar 3, 2020

Replaced by #856

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@yajo yajo yajo requested changes

@sbidoul sbidoul sbidoul approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@guewen @sbidoul @yajo