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

[python] Lint Python files with Ruff in CI (with ratchet) #26316

Merged
merged 5 commits into from
Feb 17, 2025

Conversation

jwnrt
Copy link
Contributor

@jwnrt jwnrt commented Feb 16, 2025

This PR runs Ruff in CI on all files which aren't excluded (which currently means one file).

Currently CI runs flake8 on files changed by a PR. This check is kept, Ruff runs as well.

Before this PR every single Python file fails Ruff's lints. This PR fixes one file as a proof of concept. The rest of the files are added to an exclude list and are not currently checked. The idea is that files will gradually be fixed and removed from this list, but not added (a ratchet).

I'd like to know if this is something that people want. Ruff seems like a pretty good linter and formatter that's easy to get a hold of and run.

I've tried to group these into units that are likely to be tacked as one
when working on fixing lint problems. In some cases this means a single
file, in others it's a directory or glob of directories.

Signed-off-by: James Wainwright <[email protected]>
Currently every single Python file in the repository fails Ruff lints.
Fix just one file so we can test the linter working.

Signed-off-by: James Wainwright <[email protected]>
@jwnrt jwnrt force-pushed the ruff-ratchet branch 2 times, most recently from bc333ca to 5b34efb Compare February 16, 2025 12:32
Signed-off-by: James Wainwright <[email protected]>
@jwnrt jwnrt marked this pull request as ready for review February 17, 2025 15:59
@jwnrt jwnrt requested a review from rswarbrick as a code owner February 17, 2025 15:59
@jwnrt jwnrt requested review from HU90m, machshev and a team and removed request for rswarbrick February 17, 2025 16:00
Copy link
Contributor

@rswarbrick rswarbrick left a comment

Choose a reason for hiding this comment

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

I'm a fan! This looks good to me, and thanks for doing the "giant exclusion list" approach: I think it's probably the right way to do things.

Copy link

@machshev machshev left a comment

Choose a reason for hiding this comment

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

Looks great!

@jwnrt jwnrt merged commit f97b3e9 into lowRISC:master Feb 17, 2025
42 checks passed
@@ -85,6 +85,7 @@ dependencies = [
# and bug fixes. We fix the version for improved stability and manually update
# if necessary.
"chipwhisperer@https://github.com/newaetech/chipwhisperer-minimal/archive/2643131b71e528791446ee1bab7359120288f4ab.zip",
"ruff>=0.9.6",
Copy link
Contributor

Choose a reason for hiding this comment

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

This is added to the wrong group

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oops #26349

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants