Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Configure ruff formatter in CI #24

Closed
wants to merge 2 commits into from
Closed

Configure ruff formatter in CI #24

wants to merge 2 commits into from

Conversation

danielhollas
Copy link
Collaborator

@danielhollas danielhollas commented Sep 19, 2024

This PR proposes to use a ruff formatter for the code and enforce it in the CI.
Docs for ruff formatter
setup in PyCharm
One advantage of ruff is that it is also a linter (although I am not setting up the linter here, am planning to discuss separately in a follow-up PR), so we would have a single tool for both.

Ruff uses a formatting style of another very common formatter called "black", and is quite different from what we have now. @JanosJiri if you have a strong preference for the current format, that is totally fine, but in that case you need to figure out which formater PyCharm uses in your setup so I can use it as well and we can enforce it in CI.

Besides the choice of the formatter, there are other things to be decided:

  • line length: Right now I have configured maximum line length to 130 to preserve existing format. I would personally preferred a bit shorter lines, but it is entirely up to you. @JanosJiri perhaps you can experiment locally with configuring shorter lines and see what looks best to you.
  • Do we use single or double quotes? The current code was inconsistent so I used ruff's default of double quotes, but I have no strong opinions on this, both are fine and it is configurable.

@danielhollas danielhollas changed the title WIP: Configure ruff formatter in CI Configure ruff formatter in CI Sep 19, 2024
@danielhollas danielhollas marked this pull request as ready for review September 19, 2024 15:30
@danielhollas danielhollas marked this pull request as draft September 19, 2024 23:03
@JanosJiri
Copy link
Collaborator

@danielhollas is this ready for review or do you still plan to work on it? I see you requested a review but then you changed it back to draft.

@danielhollas
Copy link
Collaborator Author

Oh yes, this is ready, (although obviously this will need to be rebased on top of main). Not sure why I changed this to draft, the main tasks are now for you to decide if the change in format is okay etc as outlined in the PR message.

@danielhollas
Copy link
Collaborator Author

I guess I changed it to draft since there are a bunch of decisions that need to be made before merging.

Copy link
Collaborator

@JanosJiri JanosJiri left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm fine with this formatting. I like a bit longer lines around 130 characters, if you are ok with that. The only other changes I noticed are using double quotes, which is fine, and having spaces around arithmetic operators, which I don't like that much visually but I found ruff can't change that, so lets keep it like that. I don't want to change my PyCharm formatting as I spent quite a long time setting it up but I can always use ruff before committing, that's not an issue. Thus, I think we can go with that.

@danielhollas
Copy link
Collaborator Author

Out of curiosity what formatter do you use in PyCharmm?

@JanosJiri
Copy link
Collaborator

Pycharm has its own formatting tool where you can configure even tiny details. It's very convenient, see https://www.jetbrains.com/help/pycharm/reformat-and-rearrange-code.html if interested.

@danielhollas danielhollas added this to the v2.0.0 milestone Sep 24, 2024
@danielhollas
Copy link
Collaborator Author

I'll close this for now, I created issue #55 in case we want to do this in the future.

@danielhollas danielhollas deleted the ruff-format branch January 23, 2025 17:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants