Switched linters, black -> ruff #334
Conversation
Removed black configs, added black configs under ruff, ran ruff check and applied safe fixes
Makefile
Outdated
| @@ -3,11 +3,9 @@ | |||
| check_dirs := optimum test bench examples | |||
|
|
|||
| check: | |||
| black --check ${check_dirs} | |||
| ruff check ${check_dirs} | |||
There was a problem hiding this comment.
AFAIK, the ruff linter only takes care of very specific formatting rules (like sorting imports).
This is why you also need to call ruff format.
See an example in another project where I only use ruff:
check:
ruff check --show-fixes ${check_dirs}
ruff format ${check_dirs} --diff
style:
ruff check ${check_dirs} --fix
ruff format ${check_dirs}Also, as you can see, when calling check I added some verbosity options.
There was a problem hiding this comment.
Thanks @dacorvo for the inputs.
Will call format as well.
Also, I think the reference repo that you mentioned (for check options) might be private? As I'm getting a code 404 on hitting the link.
There was a problem hiding this comment.
Also, I think the reference repo that you mentioned (for check options) might be private?
Ah yes, sorry. I edited my message to copy the relevant portion of the Makefile.
There was a problem hiding this comment.
Thanks!
Also, as you can see, when calling check I added some verbosity options.
Yeah, makes sense to list out the changes before applying them.
| @@ -12,6 +12,6 @@ else | |||
| pip install --upgrade --pre torch torchvision torchaudio --index-url https://download.pytorch.org/whl/nightly/cu118 | |||
| fi | |||
| # Build tools | |||
| pip install black ruff pytest build | |||
| pip install ruff pytest build | |||
There was a problem hiding this comment.
This is a legacy file I should have removed a long time ago since it is obsoleted by optional targets in pyproject.toml.
Anyway, you also need to edit the python-quality CI workflows under .github.
There was a problem hiding this comment.
Anyway, you also need to edit the python-quality CI workflows under .github.
Do we want similar verbosity flags on this one as well?
I see that currently it simply calls check :
- run: ruff check bench examples optimum test
There was a problem hiding this comment.
Yes that would be useful thank you.
There was a problem hiding this comment.
Alrighty then, done.
There was a problem hiding this comment.
The reason it might still be relevant is because ppl submitting pull-request might not be familiar with the concept of linting/formatting, but would understand immediately what this is about when looking at the failing CI logs for their pull-request.
There was a problem hiding this comment.
I see... yes, agreed.
This will enable enumeration of changes before ruff applies them.
Added listing flag on ruff check
There was a problem hiding this comment.
Almost there:
- you need to check also the format in the CI workflow.
- it seems ruff formatting is a bit different, and if I run
make checklocally using your branch, I have 21 files that would need to be reformatted (most of them are about removing one line break in class declarations).
Added format check to the CI workflow Co-authored-by: David Corvoysier <david.corvoysier@gmail.com>
Sure, but you need to apply the changes into a commit, because now the CI will fail. In retrospect I wasn't clear at all in my review: please forgive me. |
Ran ruff formatter on the project (`check_dirs` in makefile) to fix format issues.
No worries! Formatting changes have been applied. 👍 |
Removed black configs, added black configs under ruff, ran ruff check and applied safe fixes
What does this PR do?
Fixes Issue #186
Before submitting
Pull Request section?
to it if that's the case. -> Issue Switch to ruff native formatter #186
Who can review?
Anyone in the community is free to review the PR once the tests have passed. Feel free to tag
members/contributors who may be interested in your PR.