-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Replaces pylint with ruff #7438
base: master
Are you sure you want to change the base?
Conversation
Ruff should be much faster so no need to have this complex script anymore
'pep8', | ||
# spellcheck introduced in version 1.4.0 | ||
'pylint>=1.4.0', | ||
'pyenchant', |
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 are loosing spell checking here. I can bring in https://crates.io/crates/typos if desired.
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.
I think this makes sense.
As a follow up pull request, I would investigate all those pylint ignore comments. Some of them can be dropped now. |
else: | ||
# Split the string on whitespace to allow passing options in |
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.
I would prefer to disable warning that requires these changes. Often it's much more readable to have full else
branch and a if statement there. If the main if
and the else if
checks are unrelated, this visual separation is a good way to hint to a logical separation.
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.
Ok I revisit these changes after #7436
@@ -163,13 +163,13 @@ def test_compare(self): | |||
b2 = self.makeService(attach_to_master=False, name='b', serviceid=21) # same args as 'b1' | |||
b3 = self.makeService(attach_to_master=False, name='b', serviceid=20) # same id as 'a' | |||
|
|||
self.assertTrue(a == a) # pylint: disable=comparison-with-itself | |||
self.assertTrue(a == a) # noqa: PLR0124 |
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.
Is it possible to configure ruff to allow full name of the warning in places like here? The warning ID is completely nondescriptive and thus is blocker for retiring pylint.
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.
Turns out like this is not implemented in ruff. But it will be there eventually.
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.
I think it makes sense to have this PR in before ruff supports human-friendly rule names (astral-sh/ruff#1773). The speed benefits outweigh small additional confusion which will be addressed in the future anyway.
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.
I find those short labels easy to lookup in the ruff documentation: https://docs.astral.sh/ruff/rules/
Them being shorter actually makes them easier to copy.
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.
Agreed, but when reading code it's completely unclear what these numeric codes mean. Usually code is read far more times than it is written.
In any case, this PR can go after merge conflicts are resolved (do we need to rebase and just drop some commits?). We can revisit the rule names later.
Depends on #7436
Contributor Checklist:
newsfragments
directory (and read theREADME.txt
in that directory)