chore: use ruff to lint and format files#3779
Conversation
There have been a few attempts at doing these, but the efforts are stale. Since `ruff` is very popular these days and it is only a single binary and is fast, let's use that. From now on we will be able to maintain the files better, because we have a CI step testing the compliance. Closes #3073
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| runs-on: ubuntu-latest | ||
| steps: | ||
| - uses: actions/checkout@v6 | ||
| - name: Install ruff | ||
| run: pip install ruff==0.15.13 | ||
| - name: Run ruff check | ||
| run: ruff check --output-format=github | ||
| - name: Run ruff format check | ||
| run: ruff format --check |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request replaces isort and black with ruff for linting and formatting across the repository, leading to extensive automated changes in imports and code style. While the transition modernizes the project's tooling, the automated cleanup removed several imports required for tests to function correctly, specifically in system_python_nodeps_test.py, repl_test.py, and verify_files_test.py. The reviewer also identified a version typo in the pre-commit configuration and recommended lowering the target-version in ruff.toml to maintain support for older Python versions.
| @@ -8,4 +8,3 @@ | |||
| # Verify a C-implemented module can be imported. | |||
| # Socket isn't implement in C, but requires `_socket`, | |||
| # which is implemented in C | |||
There was a problem hiding this comment.
The import of socket was removed by the automated cleanup, but it is required for this test to verify that C-implemented modules can be imported. Since this file is already included in per-file-ignores for F401 in ruff.toml, the import should be restored.
| # which is implemented in C | |
| # which is implemented in C | |
| import socket |
| """Validates that we cannot import helper/test_module.py since it's not a direct dep.""" | ||
| with self.assertRaises(ModuleNotFoundError): | ||
| import test_module | ||
| pass |
There was a problem hiding this comment.
The import was replaced with pass, which will cause this test to fail because assertRaises expects an exception to be raised. The import should be restored with a # noqa: F401 comment to prevent Ruff from removing it again.
| pass | |
| import test_module # noqa: F401 |
| import somepkg.a | ||
| import somepkg.subpkg | ||
| import somepkg.subpkg.b | ||
| pass |
There was a problem hiding this comment.
These imports were removed by the automated cleanup, making the test useless. They should be restored to verify that the packages are indeed importable. Adding # noqa: F401 will prevent them from being removed in future linting runs.
| pass | |
| import somepkg # noqa: F401 | |
| import somepkg.a # noqa: F401 | |
| import somepkg.subpkg # noqa: F401 | |
| import somepkg.subpkg.b # noqa: F401 |
| @@ -0,0 +1,30 @@ | |||
| line-length = 100 | |||
| target-version = "py313" | |||
There was a problem hiding this comment.
Setting target-version to py313 indicates that the project only supports Python 3.13 and newer. This may cause Ruff to suggest or enforce syntax that is incompatible with older Python versions. It is recommended to set this to the minimum Python version supported by the project (e.g., py38 or py39).
| target-version = "py313" | |
| target-version = "py38" |
dougthor42
left a comment
dougthor42
left a comment
There was a problem hiding this comment.
Most of the AI comments are actually decent. One wasn't.
+2 when those are addressed.
3 participants
There have been a few attempts at doing these, but the efforts are
stale. Since
ruffis very popular these days and it is only a singlebinary and is fast, let's use that.
From now on we will be able to maintain the files better, because we
have a CI step testing the compliance.
Closes #3073