tests: add linter and check black#1
tests: add linter and check black#1utnapischtim wants to merge 1 commit intoinveniosoftware:masterfrom
Conversation
* ruff rules: I .. isort T20 .. p(p)rint statements F401 .. unused imports F841 .. unused variable F811 .. redefined while unused
| - uses: psf/black@stable | ||
| with: | ||
| options: "--check --verbose" | ||
|
|
||
| - name: Linter | ||
| uses: astral-sh/ruff-action@v3 | ||
| with: | ||
| args: "check --select I,T20,F401,F841,F811" |
There was a problem hiding this comment.
The issue with this is that you cannot run this locally, before pushing your changes.
That's why we have the run-tests.sh and the rules configured in the setup.cfg.
What are actually checking now with ruff?
Wasn't it black + isort enough?
There was a problem hiding this comment.
yes we have the run-tests.sh but if we change something there, we have to apply it again on every package.
introducing this change, would improve the test workflow on github without interfering locally. in my thinking the local development checks about black in the IDE by applying black on the save file hook. further isort is applied there too. so if you run run-tests.sh locally with black and isort included it should not be a problem if it runs at the end of the pytest tests.
the same configuration, as we have locally would still work on github, but would not fail anymore since it would fail before, so we are faster.
i enabled following ruff rules:
T20 .. p(p)rint statements
F401 .. unused imports
F841 .. unused variable
F811 .. redefined while unused
which are not covered by black and isort. the I stands for isort. i included that, because ruff has it included, but we can remove that and stay on isort for it. for me i would like to include those linter rules, because some of them can hide bugs unused variable and redefined while unused.
There was a problem hiding this comment.
I am not against this, but we need to be aware that it will initially fail for some repositories. I randomly ran it on Invenio-Communities
ruff check --select I,T20,F401,F841,F811
...
Found 18 errors.
No fixes available (18 hidden fixes can be enabled with the `--unsafe-fixes` option).There was a problem hiding this comment.
yes it will fail. if it would not fail, i would have not the reason to activate it!
i included only the bar minimum what i think we should enable. we can discuss afterwards if we want to introduce further rules. on my packages i do it the other way around, i enable all rules and ignore rules where i think they are to much or are not applicable to the invenio code base
3 participants
I .. isort
T20 .. p(p)rint statements
F401 .. unused imports
F841 .. unused variable
F811 .. redefined while unused
check how it works with ruff rules here
check how it works with black here
NOTE ruff rule errors are shown in
Files changedblack not yet. that should be improved!