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

Dropping isort and move to lint.isort rule in ruff #5712

Open
wants to merge 13 commits into
base: main
Choose a base branch
from

Conversation

tkoyama010
Copy link
Member

@tkoyama010 tkoyama010 commented Feb 29, 2024

Overview

Dropping isort and moving to lint.isort rule in ruff.

Close #6137

Details

@github-actions github-actions bot added maintenance Low-impact maintenance activity dependencies Pull requests that update a dependency file labels Feb 29, 2024
@tkoyama010
Copy link
Member Author

pre-commit.ci autofix

Copy link

codecov bot commented Feb 29, 2024

Codecov Report

Attention: Patch coverage is 99.26618% with 11 lines in your changes are missing coverage. Please review.

Project coverage is 97.08%. Comparing base (7841b34) to head (9ba41d6).

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #5712      +/-   ##
==========================================
+ Coverage   96.99%   97.08%   +0.08%     
==========================================
  Files         141      141              
  Lines       24669    25935    +1266     
==========================================
+ Hits        23928    25179    +1251     
- Misses        741      756      +15     

@user27182
Copy link
Contributor

I'm not sure how much overlap this tool has with isort, but this seems worth considering :

https://github.com/asottile/reorder-python-imports

Its main goal is to reduce merge conflicts.

@tkoyama010 tkoyama010 marked this pull request as draft March 1, 2024 08:47
@user27182
Copy link
Contributor

I'm not sure how much overlap this tool has with isort, but this seems worth considering :

https://github.com/asottile/reorder-python-imports

Its main goal is to reduce merge conflicts.

For context, I stumbled upon reorder-python-imports trying to look for a solution to #5621 (comment) . In this case, reorder-python-imports could be used to automatically add from future import __annotations__ to every file in the code base to support the type aliases for the docs. The config would be similar to this: https://github.com/asottile/pyupgrade/blob/df17dfa3911b81b4a27190b0eea5b1debc7ffa0a/.pre-commit-config.yaml#L18-L20

I plan on opening a new PR with reorder-python-imports to support this. And so if this works and the PR is accepted, then perhaps we may want to also use reorder-python-imports instead of isort. Just an idea though, I am not very familiar with these linting tools.

@tkoyama010
Copy link
Member Author

Just an idea though, I am not very familiar with these linting tools.

No problem. We will review it.

@user27182
Copy link
Contributor

I'm not sure how much overlap this tool has with isort, but this seems worth considering :
https://github.com/asottile/reorder-python-imports
Its main goal is to reduce merge conflicts.

For context, I stumbled upon reorder-python-imports trying to look for a solution to #5621 (comment) . In this case, reorder-python-imports could be used to automatically add from future import __annotations__ to every file in the code base to support the type aliases for the docs. The config would be similar to this: https://github.com/asottile/pyupgrade/blob/df17dfa3911b81b4a27190b0eea5b1debc7ffa0a/.pre-commit-config.yaml#L18-L20

I plan on opening a new PR with reorder-python-imports to support this. And so if this works and the PR is accepted, then perhaps we may want to also use reorder-python-imports instead of isort. Just an idea though, I am not very familiar with these linting tools.

I looked into using reorder-python-imports. It is fundamentally incompatible with black (black adds a new line after docstrings, reorder-python-imports insists on removing it), see summary about this issue here: psf/black#4175 (comment)

The main goal of my earlier suggestion was to add from __future__ import annotations to every file. This can be accomplished with ruff and the proposed changes from this PR by adding this option:

[tool.ruff.lint.isort]
required-imports = ["from __future__ import annotations"]

As an aside, in case there is a desire to reduce merge conflicts (which is another reason to use reorder-python-imports), the force-single-line option may also be used.

So, there is no need to make use of reorder-python-imports as a pre-commit hook.

+1 for this PR and dropping isort and using ruff instead.

@tkoyama010 tkoyama010 marked this pull request as ready for review May 28, 2024 23:54
@tkoyama010
Copy link
Member Author

pre-commit.ci autofix

@tkoyama010 tkoyama010 requested a review from user27182 May 29, 2024 00:02
@tkoyama010 tkoyama010 enabled auto-merge (squash) May 29, 2024 00:03
@tkoyama010 tkoyama010 disabled auto-merge May 29, 2024 00:03
@tkoyama010
Copy link
Member Author

pre-commit.ci autofix

@tkoyama010 tkoyama010 enabled auto-merge (squash) May 29, 2024 00:07
@pre-commit-ci pre-commit-ci bot requested a review from a team as a code owner May 29, 2024 00:09
@tkoyama010
Copy link
Member Author

pre-commit.ci autofix

@tkoyama010 tkoyama010 disabled auto-merge May 29, 2024 00:15
@user27182
Copy link
Contributor

The documentation build for 1fc8cfc took about 2h20 mins. Usually it's about 1h10 mins. Not sure how the caching works for the build, maybe it took extra long because pretty much every file in the repo got modified, so had to be completely re-built from scratch? Hopefully it's not a permanent increase in build time.

@tkoyama010
Copy link
Member Author

The documentation build for 1fc8cfc took about 2h20 mins. Usually it's about 1h10 mins. Not sure how the caching works for the build, maybe it took extra long because pretty much every file in the repo got modified, so had to be completely re-built from scratch? Hopefully it's not a permanent increase in build time.

The cause is that the directory under the examples directory was rerun. After the merge, a new cache will be created and we expect the execution time to return to normal.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file maintenance Low-impact maintenance activity
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants