Switch to ruff linting #2952
Switch to ruff linting #2952
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2952 +/- ##
==========================================
- Coverage 89.63% 86.65% -2.99%
==========================================
Files 29 11 -18
Lines 30189 22932 -7257
Branches 5877 4254 -1623
==========================================
- Hits 27061 19872 -7189
+ Misses 1789 1754 -35
+ Partials 1339 1306 -33
Flags with carried forward coverage won't be shown. Click here to find out more. |
benjeffery
force-pushed
the
ruff
branch
4 times, most recently
from
bd43a25
to
17706a7
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2952 +/- ##
==========================================
- Coverage 89.63% 89.63% -0.01%
==========================================
Files 29 29
Lines 30189 30173 -16
Branches 5877 5877
==========================================
- Hits 27061 27045 -16
Misses 1789 1789
Partials 1339 1339
Flags with carried forward coverage won't be shown. Click here to find out more.
|
benjeffery
force-pushed
the
ruff
branch
2 times, most recently
from
cfd0171
to
1bd604c
Compare
|
Well this was a lot noisier than I was expecting, although it did show up a couple of actual errors. |
|
Ruff is quite opinionated about getting people to use f strings etc isn't it? What were the errors? LGTM though, happy to switch. Given the noisiness we may want to try and flush a few open PRs through first. |
|
If we don't want to enforce f-strings we can switch that off - they do generally read better though. The errors were: |
| ], | ||
| ) | ||
| @pytest.mark.parametrize("mode", DIVMAT_MODES) | ||
| @pytest.mark.parametrize("span_normalise", [True, False]) | ||
| def test_all_trees_windows(self, windows, mode, span_normalise): | ||
| print(windows) |
There was a problem hiding this comment.
I only had a brief look through, but apart from a stray print, everything LGTM. Setting the line length to 90 rather than 89 has made a few lines more readable too.
Is it worth running a perf test, as there have been a few changes involving changing lists to tuples (I assume this should make things a smidge faster, although not noticeably)
| - (n + 2) / (h * n) | ||
| + g / h**2 | ||
| ) | ||
| b = 2 * (n**2 + n + 3) / (9 * n * (n - 1)) - (n + 2) / (h * n) + g / h**2 |
There was a problem hiding this comment.
I don't really like this change - the previous version was written out on multiple lines so it's easier to parse by eye. However, most of these many lines -> one line changes I do like, so maybe it's okay?
There was a problem hiding this comment.
In this one case you could #noqa the line, I guess?
| f"Valid options are {str(list(codec_registry.keys()))}." | ||
| ) | ||
| f"Valid options are {list(codec_registry.keys())!s}." | ||
| ) from None |
There was a problem hiding this comment.
This raise from None thing is going to be a bit opaque to new people.
There was a problem hiding this comment.
I fail to see the value of that one too
There was a problem hiding this comment.
It makes the output easier to read when exceptions occur, although developers may prefer to see the entire context chain of raised errors.
|
I'm happy with whatevery you all think, but I did find two places I wished it was less opinionated (see comments). |
|
I've been using ruff and it's much quicker than our previous linter, which makes development a little slicker and more palatable, so I would be happy with switching over. |
|
Still planning to do this - waiting till the number of in-flight PRs is a bit lower. |
For numpy2 checks etc.