Skip to content

Ruff autoformatting expressions/ folder#902

Draft
IgnaceBleukx wants to merge 4 commits intomasterfrom
ruff
Draft

Ruff autoformatting expressions/ folder#902
IgnaceBleukx wants to merge 4 commits intomasterfrom
ruff

Conversation

@IgnaceBleukx
Copy link
Copy Markdown
Collaborator

As discussed offline, I played around with ruff a little bit.
You can go very far in configuring its rules, some are quite pedantic imho...

This is just a PR to show how our expressions/ folder could look like if we were to add autoformatting.
I've enabled the recommended doc-styles and standard pyflakes code formatting, but enabled some rules of it as they were too incompatible with the current code base...

If we decide to go with autoformatting, there is also the question on how to integrate it continuously... Formatting the entire code base at once will destroy our git history, but after chatting with a friend, they suggested me a few options:

  1. Only format changed files when pushing new changes
  2. Only format changed lines (with git diff) when pushing new changes -- can cause some weirdness as ruff often needs more context for proper formatting
  3. Do a big formatting commit, but ignore it in the git blame history git config blame.ignoreRevsFile .git-blame-ignore-revs

We can also setup the autoformatting in the CI of course so we don't all have to do it locally.

@tias
Copy link
Copy Markdown
Collaborator

tias commented Apr 3, 2026

we're now consistently typing the code, having a consistent style with a preconfigured linter could also help in being more mature

but auto-formatting? I think we're a bit too far away from that... I see that ruff also has IDE plugins; if we each install that and when working on code, we make sure that we fix the linter warnings, then I think we have a more gradual way forward?

We are touching large parts of the code for another month anyway... and then maybe after doing it when we make code changes that we could consider autoformatting...

note that I would also be open for PRs that fix one linter warning; e.g. making sure all strings use " could be one, or starting each comment with a Capital letter, or things like that.

(the multi-line imports and function calling, I find it less readable, so I will have to get used to that more slowly)

do the IDE integrations also follow the project.toml? because then perhaps we could already include just that?

@hbierlee
Copy link
Copy Markdown
Contributor

hbierlee commented Apr 3, 2026

I wanted to add a few loose points to adding auto-formatting, since I had thought a bit about it previously:

  1. From Ignace's options, I think just picking a few rules we like and then just doing one big auto-format commit is probably the best way, ignoring it in the git blame history.
  2. Note that ruff can also --fix lint issues (say changing == False to is False in case it is sure that this is save), so there's no need to do that manually (although I'd first do regular formatting, then linting fixes in a separate commit so we can manually check it). Anything ruff can't fix safely, it will leave as linting error, which can be done manually at once too.
  3. I think going gradual could work (at least for linting, not gradual auto-formatting). In the end, I think it will just be unnecessary effort and I don't really see the upside. Now this (automatable) change becomes the responsibility of all authors, and you will need to resist not fixing warnings if they are not in lines of code you are currently working on, but this is more or less how I've been working :)
  4. Related, having many small linting PRs is I agree is less jarring (rather than a big auto-format commit), but one downside is that you will need to add all those commits to the ignore-revs list.
  5. I think the biggest downside to doing (either gradual or big) auto-formatting commits is that all outstanding PRs will get merge conflicts. I think the PR author should avoid manually resolving these, meaning they run the auto-formatter themselves after merging the project rules (a little tricky!). Having good timing (e.g. after this month) would alleviate the issue a bit.
  6. Indeed, whatever rules we check into the repo, any extension will respect them (it's just calling ruff under the hood).
  7. The usual way I see linting/formatting standardized in terms of CI is that it just fails if there are linting/formatting violations (using e.g. https://github.com/marketplace/actions/ruff-action). This sounds annoying, but I think it's fine, you can just ignore it and do a final formatting run. Alternatively, the CI could just warn (allowing merging of non-standard code), or it could fix the issues (never tried this, so not sure how smooth that workflow would be)
  8. If somebody doesn't like a rule, we can simply change it, right? If @tias doesn't like some rule, e.g. the multi-line imports. For me, the advantage of auto-formatting is the consistency you get (e.g. cleaner git diffs, better readability, no more time spent manually formatting stuff, unless you have some specific purpose in which case you can ignore formatting for blocks of code). I don't care about bikeshedding which specific rules are picked. Note the rules can always be changed later (using another big auto-formatting commit, if it's worth the aforementioned downside).
  9. Final thing I'm curious about, while the lint/formatting commit can be ignored with ignore-revs, I don't expect this to be perfect? It will be merging some lines together. Not a huge issue imo

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants