Skip to content

MNT add isort to ruff's rules #26649

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

Merged
merged 7 commits into from
Jun 21, 2023
Merged

Conversation

adrinjalali
Copy link
Member

@adrinjalali adrinjalali commented Jun 21, 2023

cc @scikit-learn/core-devs

closes #22853

Now that we have ruff and the bot, we can enable isort rules in ruff. This PR does that. Would be nice to mere this quick so that we minimize merge conflicts.

@github-actions
Copy link

github-actions bot commented Jun 21, 2023

Linting Passed

All linting checks passed. Your pull request is in excellent shape! ☀️

Copy link
Member

@glemaitre glemaitre left a comment

Choose a reason for hiding this comment

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

We only need to wait that the linter CI pass.
LGTM then.

Copy link
Member

@jjerphan jjerphan left a comment

Choose a reason for hiding this comment

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

Thank you, @adrinjalali.

This LGTM given that:

PS: I like the new bot; kudos to the people who configured it.

Copy link
Member

@jeremiedbb jeremiedbb left a comment

Choose a reason for hiding this comment

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

Finally !

@thomasjpfan thomasjpfan merged commit 42173fd into scikit-learn:main Jun 21, 2023
@adrinjalali adrinjalali deleted the ruff-isort branch June 21, 2023 16:19
@adrinjalali
Copy link
Member Author

@jjerphan hopefully not many people would have issues since this is limited to the imports section, but we can work on the docs if issues arise.

@betatim
Copy link
Member

betatim commented Jun 29, 2023

Came here while dealing with conflicts that I think are due to isort. In particular what happened is that code like this:

from sklearn.preprocessing import (
    Binarizer,
    KernelCenterer,
    MaxAbsScaler,
    MinMaxScaler,
    Normalizer,
    PowerTransformer,
    QuantileTransformer,
    RobustScaler,
    StandardScaler,
    add_dummy_feature,
    maxabs_scale,
    minmax_scale,
    normalize,
    power_transform,
    quantile_transform,
    robust_scale,
    scale,
)

has turned into the following:

from sklearn.preprocessing import Binarizer
from sklearn.preprocessing import KernelCenterer
from sklearn.preprocessing import Normalizer
from sklearn.preprocessing import normalize
from sklearn.preprocessing import StandardScaler
from sklearn.preprocessing import scale
from sklearn.preprocessing import MinMaxScaler
from sklearn.preprocessing import minmax_scale
from sklearn.preprocessing import QuantileTransformer
from sklearn.preprocessing import quantile_transform
from sklearn.preprocessing import MaxAbsScaler
from sklearn.preprocessing import maxabs_scale
from sklearn.preprocessing import RobustScaler
from sklearn.preprocessing import robust_scale
from sklearn.preprocessing import add_dummy_feature
from sklearn.preprocessing import PowerTransformer
from sklearn.preprocessing import power_transform
from sklearn.preprocessing._data import _handle_zeros_in_scale
from sklearn.preprocessing._data import BOUNDS_THRESHOLD
from sklearn.metrics.pairwise import linear_kernel

Is this expected? From the diff/example in the original issue #22853 it looked like these "grouped imports" (better name?) would be preserved.

I have two issues with this:

  1. it makes resolving the conflict very hard (I can't "just" accept the incoming change, I need to also delete code outside of the conflict and then double check that the new imports that the PR wants to add are still added).
  2. I find the new style visually very not pleasing. Beyond what I posted above which I find tedious to read (hello java!) it also moves imports from sklearn.utils._testing above imports from sklearn.preprocessing which I dislike because "more important/relevant imports should come first" or however best to describe the "style" of putting imports from stdlib first, then big global packages, then local stuff, then private things, etc.

For (1): is there a tool I can use? Can I somehow accept both changes and then run ruff to have it re-sort and remove duplicate imports?

For (2) are there config options we can use (and debate endlessly ;) ) that influence how isort does its business? I can't make a rational argument for why I prefer the "pre isort" style, but aesthetically I find it much more pleasing and "normal".

@jeremiedbb
Copy link
Member

it makes resolving the conflict very hard (I can't "just" accept the incoming change, I need to also delete code outside of the conflict and then double check that the new imports that the PR wants to add are still added).

Accepting both incoming and current and then running ruff --fix is a good solution for that.

@jeremiedbb
Copy link
Member

For (2) are there config options we can use (and debate endlessly ;) ) that influence how isort does its business? I can't make a rational argument for why I prefer the "pre isort" style, but aesthetically I find it much more pleasing and "normal".

To me, the reasoning is the same as we had for black. Some people prefer black formatting, some people prefer another formatting convention. In the end, aesthetic doesn't really matter here. The goal is just that we don't have to think about it.

@betatim
Copy link
Member

betatim commented Jun 29, 2023

Thanks for the tip for (1). Grumpy man Tim says "I've never had to think about imports until today" - back to productive work now instead of arguing on the internet :D

jeremiedbb pushed a commit to jeremiedbb/scikit-learn that referenced this pull request Jun 29, 2023
jeremiedbb pushed a commit that referenced this pull request Jun 29, 2023
REDVM pushed a commit to REDVM/scikit-learn that referenced this pull request Nov 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

RFC isort as linter and import sorter
6 participants