Conversation
heevasti
left a comment
heevasti
left a comment
There was a problem hiding this comment.
Surprisingly little changes in the end, I would have thought it would have been at least double the number. So that's good, nicely tuned! My main comments are that we should have also a badge for ruff, now that pylint badge gets dropped, and that the Ruff does not need to evaluate the unit-tests modules so no "# noqa: ..." lines should be needed there. The some code neatness changes already present can be kept, but no need to make the Ruff pass the unit-tests in the future.
| def test_parse_symbols_no_dots(self): | ||
| """Test the path is returned into a subfolder if no dots present.""" | ||
| global TEST_PROGRAM | ||
| global TEST_PROGRAM # noqa: PLW0603 |
There was a problem hiding this comment.
I see the formatter has also re-formatted parts of the testing module. It is fine for some small code improvements but these "noqa" should not be here as unit-tests should be left outside of the code formatting checks. For unit-tests it is important that they work, not how they look.
There was a problem hiding this comment.
I agree that formatting is not the issue at hand, but linting is. Note that none of these changes are due to the ruff formatter, as that was not invoked (or fully configured) for this PR.
Unittests should still be correct and ideally not bug-prone and that is what the linter is for.
The usage of globals in the unittests, for instance, is a code smell and a potential source of errors.
I think deviating from that at times can be warranted, but that is what the noqa comments are for.
I would thus like to propose we do not special-case the tests for the linter.
| @@ -1,4 +1,3 @@ | |||
| [](https://github.com/QuTech-Delft/QMI/blob/main/.github/badges/pylint.svg) | |||
There was a problem hiding this comment.
This badge should be replaced with a Ruff 'pass' or 'fail' -kind badge, or 'pass' or 'errors: n' -kind.
There was a problem hiding this comment.
Ah, didn't know that existed. I'll see how I can create something like that. However, is it useful to have such a badge if, much like mypy, any error will cause the pipeline to fail and thus prevent a merge into main?
There was a problem hiding this comment.
I created a badge that shows the amount of ruff errors. What do you think?
| git fetch origin "${{ github.head_ref }}" || true | ||
| git checkout "${{ github.head_ref }}" | ||
| pip install anybadge | ||
| anybadge -o -l pylint -v $(tail -n 2 pylint-3.11.log | grep -o '[0-9]\{1,2\}\.[0-9]\{2\}' | head -n 1) -f $BADGES_DIR/pylint.svg 2=red 4=orange 8=yellow 10=green |
There was a problem hiding this comment.
There should be a ruff badge that gets created in place of the pylint badge. A simple 'pass' | 'fail' badge should suffice for Ruff, as it does not really produce a "score". Alternatively, a 'pass' or 'errors: n' kind of badge.
Thom747
force-pushed
the
replace-pylint-with-ruff
branch
from
f72fbcd to
a628477
Compare
2 participants
This PR replaces the slow Pylint formatter with Ruff.
As Ruff does not output a score, but just fails on any errors, this PR also made the requirement on the amount of linting errors more stringent: from a Pylint score of 9.0 to 0 errors.