GitHub Action to lint Python code with ruff #718
Conversation
cclauss
force-pushed
the
ruff
branch
2 times, most recently
from
0fc56b3
to
1d4199c
Compare
.github/workflows/ruff.yml
Outdated
| steps: | ||
| - uses: actions/checkout@v3 | ||
| - run: pip install --user ruff | ||
| - run: ruff --format=github . |
There was a problem hiding this comment.
The GitHub code annotations will conduct by adding the --format=github option? I did not know about this feature yet, thank you so much! Ruff is an excellent piece of software, we love it.
/cc @charliermarsh
There was a problem hiding this comment.
There was a problem hiding this comment.
💡 You can also use the RUFF_OUTPUT_FORMAT environment variable, which is useful if ruff is embedded in e.g. a pre-commit flow:
run: pre-commit run --all-files
env:
RUFF_OUTPUT_FORMAT: github
There was a problem hiding this comment.
Is the --format still supported by ruff ? When I run it locally, I have:
error: unexpected argument '--format' found
I guess it was renamed --output-format
There was a problem hiding this comment.
It was renamed in September (astral-sh/ruff#7514) and the old name deprecated in October (astral-sh/ruff#7984).
This PR has been pending since April... 😁
|
@PierreF Your review, please. |
.github/workflows/lint-python.yml
Outdated
| push: | ||
| branches: [master] | ||
| pull_request: | ||
| branches: [master] |
There was a problem hiding this comment.
This filter shouldn't be necessary; otherwise this workflow won't be run on PRs that don't target master.
There was a problem hiding this comment.
Without the filter, maintainers will double-test all pull requests to master. The first ten open PRs target master and my bet is that trend will continue.
There was a problem hiding this comment.
I just cause few conflicts by pushing on master.
Could you also sign-off your commit ? It's the "-s" option on git commit CLI. Or it's just adding the "Signed-off-by: YOUR-NAME-AND-EMAIL" in commit message. Eclipse require this to confirm you agree with the ECA that you already signed.
| # which does not support asyncio and type hints | ||
| flake8 . --count --select=E9,F63,F7,F822,F823 --show-source --statistics {env:EXCLUDE:} | ||
| python setup.py test | ||
| make -C test test |
There was a problem hiding this comment.
Why drop the execution of those tests ? I don't think they are run by something else.
There was a problem hiding this comment.
Those tests should be converted to be regular py.test tests IMO, they now seem to rely on some very homespun code (e.g. some Python code in .test files that maybe somehow relates to the .py files...).
They're also not covered by any code coverage so long as that's happening.
There was a problem hiding this comment.
Agree, but they are not yet converted, so in meantime it seems better to kept them.
There was a problem hiding this comment.
make -C test test might be useful but I was unable to make it work.
Calling setup.py xxx is deprecated because it was a great way to hack in malware. So, let's allow pytest test discovery to find and run the tests (see below).
pytest can run old-style unit tests out-of-the-box so while converting tests might be useful, it is not obligatory.
py.test (with the dot) was deprecated years ago.
There was a problem hiding this comment.
PR #768 will re-introduce test lost from the make -C test
.github/workflows/ruff.yml
Outdated
| steps: | ||
| - uses: actions/checkout@v3 | ||
| - run: pip install --user ruff | ||
| - run: ruff --format=github . |
There was a problem hiding this comment.
Is the --format still supported by ruff ? When I run it locally, I have:
error: unexpected argument '--format' found
I guess it was renamed --output-format
Signed-off-by: Christian Clauss <cclauss@me.com>
Signed-off-by: Christian Clauss <cclauss@me.com>
| - uses: actions/checkout@v2 | ||
| - uses: actions/setup-python@v2 | ||
| - run: pip install tox | ||
| - run: tox -e lint |
There was a problem hiding this comment.
Kept the run command as install tox & tox -e lint.
This allow to run the same linter locally
| flake8<4 | ||
|
|
||
| # This lint environment should be the same as the one in .github/work | ||
| [testenv:lint] |
There was a problem hiding this comment.
Kept the lint environment, this allow to run linter locally. I want to be able to run lints before pushing to github.
The idea is the the tox contains the same linter that are currently present in github workflow, so locally or Github could call tox -e lint to check linters.
I've made few change with what was run by Github lint action, mostly instead of . I use src. This was because without it, linters would process the .tox folder which is quiet large.
| deps = | ||
| -rrequirements.txt | ||
| flake8 | ||
| allowlist_externals = |
There was a problem hiding this comment.
We can drop the allowlist_externals. We only use pytest in commands which is installed by tox (so not external)
| matrix: | ||
| python: [3.7, 3.8, 3.9] | ||
| python: ["3.8", "3.9"] # TODO: Add these... , "3.10", "3.11", "3.12"] |
There was a problem hiding this comment.
You can add them, but upgrade pytest in requirements.txt to latest version (7.4.3).
| # which does not support asyncio and type hints | ||
| flake8 . --count --select=E9,F63,F7,F822,F823 --show-source --statistics {env:EXCLUDE:} | ||
| python setup.py test | ||
| make -C test test |
There was a problem hiding this comment.
PR #768 will re-introduce test lost from the make -C test
Failing tests are fixed in #712
Ruff supports over 500 lint rules and can be used to replace Flake8 (plus dozens of plugins), isort, pydocstyle, yesqa, eradicate, pyupgrade, and autoflake, all while executing (in Rust) tens or hundreds of times faster than any individual tool.
The ruff Action uses minimal steps to run in ~5 seconds, rapidly providing intuitive GitHub Annotations to contributors.