Try out ruff
#227
Try out ruff
#227
Conversation
anatoly-scherbakov
requested review from
BigBlueHat,
davidlehn and
mielvds
and removed request for
mielvds
mielvds
left a comment
mielvds
left a comment
There was a problem hiding this comment.
Big supporter of this; I use ruff myself. Flake8 is also fine, as long as there is some formatter. Ruff includes flake8 & beyond, so obvious choice
|
@anatoly-scherbakov maybe one question: is the config aligned with the one from rdflib? |
|
@mielvds not sure, can look into a comparison 👀 It won't be a 100% match since |
|
@mielvds I have checked this out and found two particular differences, fixed. |
davidlehn
left a comment
davidlehn
left a comment
There was a problem hiding this comment.
- I wasn't familiar with ruff, but if that's a preferred tool, and flake8-ish compatible, that's fine.
- As noted, may be better to leave the line length issue until a later single commit if needed.
- Not a fan of the commits with plus signs and arrows. I'd prefer accessible clear words such as "add" and "with" vs trying to be cute. The issue id doesn't really need to be in the main commit summary lines either. And one commit has an errant backslash.
| 'jsonld.InvalidUrl', | ||
| {'url': url, 'cause': cause}, | ||
| code='loading remote context failed') | ||
| code='loading remote context failed') from cause |
| # At this stage, we are limiting our formatting efforts to one file. We need to ensure that: | ||
| # * our formatting rules are sane, | ||
| # * we are not introducing conflicts with currently open PRs, | ||
| # * and the PR introducing `ruff` is not too large. | ||
| RUFF_TARGET = lib/pyld/context_resolver.py |
There was a problem hiding this comment.
- This contemporary commentary isn't needed. Looks more like PRs comments than code ones.
- Needs to point at everything (as before) before merging, at least for the check part.
- I assume checks can't be too bad, since it was passing flake8 before. If it is, maybe tune/comment the config temporarily until more involved changes can be addressed?
There was a problem hiding this comment.
I agree with these points. I guess Anatoly added this commentary for our reviewing convenience, but it should go before merging. Let's also add the other files as well (after rebase!) and discuss the formatting as a whole.
For the last point though: could we turn this around and see how involved the changes actually are? It might be unnecessary to do this incrementally, which will save us some time.
| target-version = "py310" | ||
|
|
||
| # Line length (matching rdflib's flake8 config) | ||
| line-length = 88 |
There was a problem hiding this comment.
I assume a reformat with this would be a massive style only change. Might be good to leave at flake8 settings until it's desired (if ever) to reformat everything as one single pass.
There was a problem hiding this comment.
@anatoly-scherbakov This is a good point. Opposed to what I wrote earlier, I suggest we put this line in comments (with a TODO note to switch when ready) and set it to 79 for now.
Why
We do not have a code formatter that would help us unify the codebase.
What
Implement
ruff. Run it in CI, but only against one file: to taste it and to reduce the probability of git conflicts.