Skip to content

Conversation

@p0deje
Copy link
Member

@p0deje p0deje commented May 28, 2025

Use Ruff directly to format and lint Python source code files.

@p0deje p0deje requested a review from cgoldberg May 28, 2025 23:59
@selenium-ci selenium-ci added C-py Python Bindings B-build Includes scripting, bazel and CI integrations labels May 28, 2025
Copy link
Member

@cgoldberg cgoldberg left a comment

Choose a reason for hiding this comment

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

Do you need to add --exit-non-zero-on-fix to check, and --exit-non-zero-on-format to format? We want the CI job to fail if it hits any violations.

Other than that, LGTM

@p0deje
Copy link
Member Author

p0deje commented May 29, 2025

Do you need to add --exit-non-zero-on-fix to check, and --exit-non-zero-on-format to format

Our CI job runs the format and then checks for git diff, so it will fail.

@cgoldberg
Copy link
Member

@p0deje There are some issues that ruff won't auto-fix (like lines that are too long)... so git diff wouldn't see those. You might want to add those args to make sure it fails.

@p0deje
Copy link
Member Author

p0deje commented May 29, 2025

@cgoldberg Hmm, I assumed that when Ruff cannot fix/format, it would fail with non-zero exit code. At least this is how I read the flag name. Will update.

@cgoldberg
Copy link
Member

I'm not 100% sure what the exit code is when it can't fix... I'd have to try, but I suppose it doesn't hurt to add the flags.

@p0deje
Copy link
Member Author

p0deje commented May 29, 2025

I did some testing and ruff seems to exit non-zero when it cannot autofix:

$ ruff check --fix scripts/pinned_browsers.py; echo $status
scripts/pinned_browsers.py:208:5: E741 Ambiguous variable name: `l`
    |
206 |     mac = None
207 |     mac_hash = None
208 |     l = "AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA…
    |     ^ E741
209 |
210 |     for data in all_data:
    |

scripts/pinned_browsers.py:208:5: F841 Local variable `l` is assigned to but never used
    |
206 |     mac = None
207 |     mac_hash = None
208 |     l = "AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA…
    |     ^ F841
209 |
210 |     for data in all_data:
    |
    = help: Remove assignment to unused variable `l`

Found 2 errors.
No fixes available (1 hidden fix can be enabled with the `--unsafe-fixes` option).
1

For format, there seem to be no cases when it would exit non-zero and not format files.

I am going to merge this as-is, we should be safe even w/o the flags.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

B-build Includes scripting, bazel and CI integrations C-py Python Bindings

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants