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

[code-style] Introduce ruff as an isort replacement and add a pre-commit hook #2619

Merged
merged 15 commits into from
Apr 11, 2024

Conversation

mbrulatout
Copy link
Contributor

Introduce a simple pre-commit hook with ruff as isort replacement.
This is a first step towards a broader ruff adoption; potentially.

Ruff has a slightly different code style, hence a few changes in the source code.
See https://docs.astral.sh/ruff/faq/#how-does-ruffs-import-sorting-compare-to-isort

Copy link
Contributor

@falkoschindler falkoschindler left a comment

Choose a reason for hiding this comment

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

Thanks for the pull request, @mbrulatout!
Currently it kind of conflicts with isort from the VSCode configuration. I tried adjusting it with

  "isort.args": ["--line-length", "120", "--multi-line", "3"]

which doesn't add a trailing comma, and

  "isort.args": ["--line-length", "120", "--multi-line", "3", "--trailing-comma", "true"]

which doesn't work at all.

I'd love to keep multiple imports on one line, because that's how it is for a small number of inputs, so why splitting them just because they don't fit on a single line. But this doesn't seem to be supported yet: astral-sh/ruff#2600).

Maybe we can completely remove isort from VSCode and use ruff instead. This would improve speed and consistency. Could you look into it, or are you working with a different IDE, @mbrulatout?

@mbrulatout
Copy link
Contributor Author

I use Pycharm but occasionally run VSCode.
I'll look into it, I know ruff has VSCode support.

…mit hook

Introduce a simple pre-commit hook with ruff as isort replacement.
This is a first step towards a broader ruff adoption; potentially.
@mbrulatout
Copy link
Contributor Author

mbrulatout commented Apr 8, 2024

@falkoschindler : got busy at work, but this should fix the VSCode integration. I'm using a different IDE most of the time, but I did try with VSCode to edit a file, save it and imports were sorted automagically.

@falkoschindler falkoschindler self-requested a review April 8, 2024 13:15
Copy link
Contributor

@falkoschindler falkoschindler left a comment

Choose a reason for hiding this comment

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

Thanks for the update, @mbrulatout!

There are, however, still some inconsistencies:

  1. If all goes well, the PR should contain (almost) no diff for imports, but it does (e.g. keyboard.py).
  2. When saving the file manually, the imports are moved into 2 lines with a length of less than 80. But with isort we're using a line length of 120.
  3. When running ruff check . --fix as described in CONTRIBUTING.md, the imports are moved back into a single column.

Maybe someone with with experience in ruff and VSCode can jump in and help fixing the configuration?

@falkoschindler falkoschindler added the help wanted Extra attention is needed label Apr 8, 2024
@mbrulatout
Copy link
Contributor Author

mbrulatout commented Apr 8, 2024

If all goes well, the PR should contain (almost) no diff for imports, but it does (e.g. keyboard.py).

The changes introduced are expected. That's a behavior difference between ruff and isort. The same thing applies when changing black to ruff format. There are changes.

Here's ruff behavior for sorting imports :

# >120 chars line
from ..events import (GenericEventArguments, KeyboardAction,  KeyboardKey, KeyboardModifiers, KeyEventArguments,
                      handle_event)

will become

from ..events import (
    GenericEventArguments,
    KeyboardAction,
    KeyboardKey,
    KeyboardModifiers,
    KeyEventArguments,
    handle_event,
)

whereas

# <120 chars line
from ..events import (GenericEventArguments, KeyboardAction,  KeyboardKey, 
                      handle_event) 

will become

from ..events import GenericEventArguments, KeyboardAction, KeyboardKey, handle_event

When saving the file manually, the imports are moved into 2 lines with a length of less than 80. But with isort we're using a line length of 120.

See behavior above

When running ruff check . --fix as described in CONTRIBUTING.md, the imports are moved back into a single column.

Not sure I understand this. Running ruff check . --fix on the current stack of commit produces no diff. Playing with a few import lines will get you into one of the two scenarii above.

@falkoschindler
Copy link
Contributor

Ah, thanks for the clarification. I was a bit confused because I thought we could mimic the isort behavior with ruff (or vice versa). But I already noticed that this is probably not possible.

I also found out why VSCode wasn't using ruff on save. We need to enable it in the workspace settings.

@falkoschindler
Copy link
Contributor

@mbrulatout There seems to be only one last open issue: When re-ordering some imports and saving without auto-formatting, I'd expect a git commit to fail or at least to output a ruff error. But I can simply commit "invalid" files. Am I doing something wrong? Or is the pre-commit hook not configured correctly?

@mbrulatout
Copy link
Contributor Author

Thanks for your commits.

Indeed you need to install pre-commit once through pre-commit --install !

@mbrulatout
Copy link
Contributor Author

mbrulatout commented Apr 8, 2024

argh I messed up the PR

Should be good now

# Conflicts:
#	website/documentation/content/section_data_elements.py
#	website/documentation/content/storage_documentation.py
Copy link
Contributor

@falkoschindler falkoschindler left a comment

Choose a reason for hiding this comment

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

Ok, I took the opportunity to add some more pre-commit hooks so that this PR actually generates some value besides paving the way for more ruff adoption. The diff got quite large, mainly caused by ruff's isort flavor, trimmed whitespace and fixed end-of-files.

Let's merge! 🙂

@falkoschindler falkoschindler merged commit 94cc62c into zauberzeug:main Apr 11, 2024
1 check passed
@falkoschindler falkoschindler added this to the 1.4.21 milestone Apr 11, 2024
@mbrulatout mbrulatout deleted the pre-commit branch April 11, 2024 17:20
@mbrulatout
Copy link
Contributor Author

Oh thanks, I also noticed you've been adding rules progressively, I wanted to do that in subsequent PRs.
I'm also having doubts about some of the pre-commit which have been added. Some of them are probably duplicate of some ruff rules.

@falkoschindler
Copy link
Contributor

Yes, once we started to get a hang of pre-commit, we added some more checks we're already using in other projects. In retrospect I should have created a pull request to give it a bit more room for discussion, but somehow I started with something noncritical like trimming whitespace and added "just one more check" until I ended up with the current collection. Of course we can discuss and potentially remove or replace every single one of them.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Extra attention is needed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants