-
Notifications
You must be signed in to change notification settings - Fork 37
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
Switch to ruff #171
Switch to ruff #171
Conversation
.pre-commit-config.yaml
Outdated
args: | ||
- --disable=consider-using-f-string,duplicate-code,missing-docstring,invalid-name,protected-access,redefined-outer-name | ||
- id: ruff | ||
args: ["--select", "I", "--fix"] |
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.
--fix
is a good one since it will help with local development, but it might help to add the rules to pyproject.toml
: (example: https://github.com/tekktrik/circfirm/blob/ab296f8a33db1ceab0b0e5f1fbeb1dc89923396c/pyproject.toml#L82-L84)
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.
All for what ever rules make sense. Was trying to keep things simple for the example
# | ||
# SPDX-License-Identifier: Unlicense | ||
|
||
* text eol=lf |
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.
Is the .gitattributes
for helping with Windows? I haven't had an issue with ruff
and EOLs.
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.
It is for helping with windows, not specifically related to ruff
but was discussed in the same larger conversation during the meeting.
Though my understanding is this was a fix for something that pylint complained about, perhaps with ruff replacing pylint we may not need this change any 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.
This will make sure they commit fine, even if they forget to run pre-commit, allowing small changes to still pass CI
.pre-commit-config.yaml
Outdated
- repo: https://github.com/charliermarsh/ruff-pre-commit | ||
rev: v0.1.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.
This appears to automatically redirect to https://github.com/astral-sh/ruff-pre-commit
I assume Github will keep that redirect active, but it's kind of an undocumented oddity for anyone that runs across it looking for the current name. I would recommend changing here to the current URL instead of relying on the redirect.
I noticed they are also up to release 0.3.2
, is there any reason why we would need to use 0.1.3 instead of the newer / current release?
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.
Based on comments, I grabbed what MicroPython was using. Totally forgot to mention that.
I created a similar draft over here in Display_Text library: adafruit/Adafruit_CircuitPython_Display_Text#204 to get my head wrapped around the changes and see another example of what kinds of stuff it might be wanting to change. The vast majority of changes in display text were tweaks to import order and extra whitespace lines sprinkled it breaking up imports and comments. I've not seen anything in the code that looks like it would be problematic on CircuitPython but I do still intend to test a few of these libraries post ruff on a device. |
Also saw some trailing comma changes, which I believe you can turn off (although I like them, because future changes are more clear) |
Not strictly related to ruff, but something else that would be nice to check for during the same automation is confirming the docs conf.py setting for |
No description provided.