-
-
Notifications
You must be signed in to change notification settings - Fork 25.8k
MNT Update ruff #30976
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
MNT Update ruff #30976
Conversation
Could you please also check that |
Sure, I'll update to ruff 0.11.0 too while I'm at it. |
f78034c
to
d2ab2d2
Compare
I applied pre-commit to the whole code base, but fixes are actually unrelated to the initial PR. $ pre-commit run -a
check yaml...............................................................Passed
fix end of files.........................................................Passed
trim trailing whitespace.................................................Passed
ruff.....................................................................Passed
black....................................................................Passed
mypy.....................................................................Passed
cython-lint..............................................................Passed
prettier.................................................................Passed
$ |
By the way, why not run pre-commit during CI? |
I've been thinking the same. I wouldn't be opposed to that actually. |
@@ -112,7 +112,7 @@ def test_make_classification_informative_features(): | |||
(2, [1 / 2] * 2, 2), | |||
(2, [3 / 4, 1 / 4], 2), | |||
(10, [1 / 3] * 3, 10), | |||
(int(64), [1], 1), | |||
((64), [1], 1), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
((64), [1], 1), | |
(64, [1], 1), |
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed, but these are automatic fixes and neither ruff
nor black
seem to be catching this one. But then see my comment #30976 (comment). I suggest deferring pre-commit fixes to a different PR anyway.
sklearn/externals/_arff.py
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we should exclude everything in externals
from linters.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ignore lists are inconsistent between tools such as black
and ruff
and pre-commit
:
Lines 112 to 125 in d2dbcff
exclude = ''' | |
/( | |
\.eggs # exclude a few common directories in the | |
| \.git # root of the project | |
| \.mypy_cache | |
| \.vscode | |
| build | |
| dist | |
| doc/_build | |
| doc/auto_examples | |
| sklearn/externals | |
| asv_benchmarks/env | |
)/ | |
''' |
scikit-learn/.pre-commit-config.yaml
Lines 14 to 17 in d2dbcff
- repo: https://github.com/psf/black | |
rev: 24.3.0 | |
hooks: | |
- id: black |
The problem is that pre-commit
will explicitly run black
, ruff
and other tools on each individual file, without taking into account the filters in pryproject.toml
. What happens then depends on whether tools such as black
or ruff
skip an explicit file argument if the file is part of its ignore list. Clearly black
does not (psf/black#438?):
$ black sklearn/externals/_arff.py
reformatted sklearn/externals/_arff.py
All done! ✨ 🍰 ✨
1 file reformatted.
$
I suggest deferring pre-commit fixes to a different PR: pre-commit is currently broken and fixing it is unrelated to the initial intent of this PR.
A maintainer should authorize |
Also, |
2b1bf4e
to
41f1acb
Compare
I have reverted the pre-commit fixes. Please follow-up in #31013. |
41f1acb
to
0267935
Compare
The linter really needed an update.
ce35f28
to
e992b1e
Compare
RUF007 Prefer `itertools.pairwise()` over `zip()` when iterating over successive pairs
RUF040 Non-string literal used as assert message
RUF046 Value being cast to `int` is already an integer
It will infer the Python version from pyproject.toml itself: requires-python = ">=3.10"
e992b1e
to
650d5c9
Compare
I meant running |
What does this implement/fix? Explain your changes.
0.9.100.11.0Any other comments?