-
-
Notifications
You must be signed in to change notification settings - Fork 8.2k
all: GitHub Action to lint Python code with ruff. #10977
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
Conversation
Code size report:
|
Idea is ok, but just like for #10911 this should have a separate configuration file so users can run the check locally (manually, or as part of a pre-commit hook). Perhaps it's time to adopt pyproject.toml? codespell, ruff, ... all support it |
Added I used |
That's pretty neat. @dpgeorge what do you think, go for pyproject.toml or not? |
Yes that looks good, to keep config all in one file. But:
|
pyproject.toml
Outdated
"F632", | ||
"F811", | ||
"F821", | ||
"F841", |
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.
Will these ignores gradually be removed once the problems are fixed?
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.
✅
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.
Small is good. As long as they are forward steps!
|
I'm not sure what 'works with IDEs' entails. Maybe because I've never used full-blown Python IDEs, but for everything else the way it works is that the 'host aplication' i.e. text editor just invokes the tools (ruff/isort/black/pylint/...) with the working directory set appropriately and by passing the path to a directory or file to process, and then the tool itself takes care of finding pyproject.toml when it's in a directory up from that path. And if it's not, the host application normally has a way to configure the tool so the user can pass it That being said it is obviously a lot more convenient and expected to have pyproject.toml (and others like .editorconfig etc) in the project root. |
What is the correct |
Codecov Report
📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more @@ Coverage Diff @@
## master #10977 +/- ##
=======================================
Coverage 98.50% 98.50%
=======================================
Files 155 155
Lines 20549 20549
=======================================
Hits 20241 20241
Misses 308 308 Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
Best thing to do is check existing commit messages. Maybe "top: Add linter ..." or "all: Reformat ...". |
5d29600
to
ebb86ee
Compare
I just spotted this https://beta.ruff.rs/docs/configuration/#pyprojecttoml-discovery |
@Josverl I believe that the place for ruff-vscode issues is at https://github.com/charliermarsh/ruff-vscode Perhaps the raw string vs. |
@cclauss , I don't think I have multiple versions (bare container image with one install of ruff). Just to be clear, I think :
|
I do not sense this is currently the case for
I am unclear which other tools this refers to. Ruff was designed to be highly compatible with psf/black. How about this approach:
|
Claus, I'm not trying to make this a battle between one or the other, but in this PR you are proposing a change. My thoughts as I outlined above , are just that , my thoughts on that proposed change. I'm no more a contributor than you are , I we both trying to help things along.
Thanks , would make sense documenting that that as a comment or otherwise for the less ruff-knowledgable such as myself.
Black is already in the pre-commit hooks, but still could benefit from a repos:
- repo: local
hooks:
- id: codeformat
name: MicroPython codeformat.py for changed files
entry: tools/codeformat.py -v -f
language: python
... |
@cclauss can you please rebase this on latest master, which now includes |
.github/workflows/ruff.yml
Outdated
- main | ||
pull_request: | ||
# branches: | ||
# - main |
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 these comments can be removed. Also, the main branch here is called master
.
Maybe just:
on: [push, pull_request]
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 that on: [push, pull_request]
will make duplicate runs only for people who have write permissions to the repo (i.e. maintainers) so I tend to use
on:
push: [master]
pull_request: [master]
but let's keep it simple given the speed of these runs.
.github/workflows/ruff.yml
Outdated
@@ -0,0 +1,16 @@ | |||
# https://docs.github.com/en/actions/automating-builds-and-tests/building-and-testing-python | |||
name: ruff |
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'd suggest a more descriptive title, eg Python code lint with ruff
pyproject.toml
Outdated
|
||
[tool.ruff.per-file-ignores] | ||
"ports/cc3200/boards/make-pins.py" = ["F524"] | ||
"shared/memzip/make-memzip.py" = ["E721"] |
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.
Can these exceptions be removed now that those files are fixed?
"F821", | ||
"PLC1901", | ||
] | ||
line-length = 337 |
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.
Shouldn't this match black's line length (99) ?
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.
The 99 is aspirational whereas the 337 is actual.
psf/black is unwilling to wrap all lines (comments? docstrings? strings?) so those lines need to be wrapped manually.
Ideally, both should be 99 once some manual work is done.
% ruff --select=E501 --line-length=99 --show-source .
[ ... ]
Found 96 errors.
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.
Hmm then why ruff says "Ruff is compatible with Black out-of-the-box, as long as the line-length setting is consistent between the two." Can you point to an example where ruff complains? There's no way to make it ignore comments for instance (should that be the culprit)?
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.
@charliermarsh, @dhruvmanila This question has come up a few times in my travels...
Do you have a better explanation? Should I create a PR to the ruff docs to improve the explanation there?
I am OK with the behavior of the two tools as they are because I believe that the goals of a formatter are different than the goals of a linter.
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.
Hmm, yeah, I'd explain it as: Black makes a best-effort attempt to wrap lines under the specified line length, but doesn't guarantee that all lines will be under the specified line length. But Ruff's rule doesn't discriminate, it just flags all lines over the limit.
You're right that in this particular case, we do arguably "disagree" with Black, depending on how you look at it. Projects with Black enabled could choose to disable this rule entirely if they're happy with Black's line-length handling, though in my personal opinion it still makes sense to enable it. A few examples of code that Black won't wrap, but Ruff will flag:
# Here's a really long comment. Black doesn't wrap comments at all. It wraps docstrings, but it
# doesn't wrap comments.
x = here_is_a_really_really_really_really_really_really_really_really_really_really_long_function_name(
1, 2, 3
)
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.
Thanks @charliermarsh for the explanation.
Sounds like we need to keep this at 337, for now. Maybe in the future we can tidy up those scripts that go over the 99 length Black limit, so this number here can be reduced.
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 clear.
Blocked by micropython#10977 This does not align with the other pre-commit jobs with are local custom code but it should run accurately and quickly once micropython#10977 has been merged.
I've only recently started using You might also want to add the setting |
Good one! We are currently ignoring % Looking at the output of |
Ah, yes as an aside I dislike the old "error number" approach of most linters, ruff included. I saw you had excluded error codes but didn't know what they were! I've been working with mypy too in my project and it has descriptive errors instead which is much more readable. But yes I'm sure there likely are more builtins worth registering... that being said most things come from imports, not new "untraceable" built-in terms like const (which technically you can import from micropython, but don't have to) If the rest of those errors are from imports that ruff can't introspect, then we might be stuck ignoring them until there are type stubs officially available for all/most built-in libraries like machine etc. |
All uses of |
Oh, I've often seen/used const without an import and assumed that was normal, just like other builtins such as print. In my current project I'm using one of the stubs packages from https://micropython-stubs.readthedocs.io/en/main/ which is probably why I'm not seeing to many other missing import errors. |
Signed-off-by: Damien George <damien@micropython.org>
I added an exclusion of the |
What about something like:
That would drop undefined names from 479 down to 66. Are there_builtins_ to add or remove? Personally, I like requiring imports because linters won't be the only ones confused about where these names are defined. |
Many of those terms you've listed come from viper or inline assembly, I wonder if there's a way to exclude functions decorated as such... probably not. Others are from manifest files, which could be excluded by file pattern perhaps? |
That's quite an ad-hoc list of built-ins, and still doesn't solve F821 completely. I would much rather use stubs. |
I've merged this because it's a good step forward, adding ruff to the CI. Moving forward, we can fix individual errors one by one. Also, would be great to add ruff to |
Ruff supports over 500 lint rules including
flake8, isort, pylint, and pyupgrade and is written in Rust for speed.
The Action uses minimal steps to run in 7 seconds, rapidly providing contributors with intuitive
GitHub Annotations.