Skip to content

Conversation

greglucas
Copy link
Contributor

Currently we use the ruff linter for style checks, but we don't have any explicit formatter. I'm proposing here that we also use the ruff formatter (drop-in replacement for black). I personally have learned to appreciate the opinionated auto-formatters because then as a reviewer I don't have to comment on minor formatting issues and as a coder I can just let the auto-formatter do its thing.

Most of the changes are single-quote to double-quote and adding a space after import lines / docstring lines. In general, I would say that most of the updates are improvements, there are a few that I maybe wouldn't have done but in my opinion that is OK and we can just let the formatter do its thing and handle the wrapping etc for us (they are still easy enough for a human to parse what goes where).

@QuLogic
Copy link
Member

QuLogic commented May 26, 2025

ruff can be set to single quotes, if that simplifies the diff (and is my personal preference, tbh.)

@greglucas
Copy link
Contributor Author

ruff can be set to single quotes, if that simplifies the diff (and is my personal preference, tbh.)

It does seem like we have more single-quote to begin with so it probably does shrink it a little bit, but not a crazy amount. I'm fine doing it either way and for this project it probably does make more sense because there are JSON strings in geometry files that use double-quotes where it is nicer to not escape those.

One interesting thing is that the sort all pre-commit hook didn't like this change because it must auto-format its sort with only double quotes by default and then the ruff formatter came back and tried to change them to single-quotes again. I've added that RUF022 rule instead for consistency.

@rcomer
Copy link
Member

rcomer commented May 27, 2025

Can we do anything about the one-entry-per-line rule for examples like this?

@greglucas
Copy link
Contributor Author

Can we do anything about the one-entry-per-line rule for examples like this?

Yes! There is a fmt: skip and fmt: off options we can apply. I added that to the examples that looked worse to me.

There are some borderline cases like in tests where there are some lists that are somewhat long, but it isn't obviously worse like the example you showed. test_eckert is a good example of the borderline cases and there are only a few files with things like that. I decided not to skip that formatting and just go with it, but happy to skip formatting those tables if anyone would prefer.

@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

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

Successfully merging this pull request may close these issues.

4 participants