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 import sorting and drop isort support #2758

Open
wants to merge 10 commits into
base: develop
Choose a base branch
from

Conversation

deen13
Copy link
Collaborator

@deen13 deen13 commented Apr 23, 2024

Short description

This pull request enables import sorting for ruff and tick some of the remaining boxes in #2442.

Side effects

While Ruff claims to be nearly equivalent to Isort when profile = "black", it formats the imports slightly differently. This pull request removes support for isort and deprecates black in our documentation to address the conflicting formatters.

Further Information: https://docs.astral.sh/ruff/faq/#how-does-ruffs-import-sorting-compare-to-isort


Pull Request Review Guidelines

@deen13 deen13 requested a review from a team as a code owner April 23, 2024 13:09
Copy link

codeclimate bot commented Apr 23, 2024

Code Climate has analyzed commit 9b3aa16 and detected 0 issues on this pull request.

The test coverage on the diff in this pull request is 100.0% (50% is the threshold).

This pull request will bring the total coverage in the repository to 82.1% (0.3% change).

View more on Code Climate.

@PeterNerlich
Copy link
Contributor

PeterNerlich commented Apr 23, 2024

I don't think tracking .vscode/ is a good idea. Could we do something like putting it into example-configs/ or otherwise renaming it but have it still feel intuitive (so you don't set up your whole project and notice at the very end that you could have based it on the default config)?

@deen13
Copy link
Collaborator Author

deen13 commented Apr 23, 2024

Hey @PeterNerlich, I don't see any negative consequences in configuring the formatter and linter properly right away. When utilized, it ensures a consistent code style, which might not adhere to the default code settings, but the CI will ultimately ensure our code style anyway. Why do you think it's better not to change the default settings?

@charludo
Copy link
Contributor

Hm, I'm with @PeterNerlich on this. Not sure we should be adding editor-specific configs. Wouldn't most people have set this up globally in their editor already anyways (and if not, probably prefer it that way)?

@PeterNerlich
Copy link
Contributor

My concern was mostly about some user changing settings and then accidentally committing them to the project. This wouldn't happen if the config in the repo was under some other name (like vscode.example/) that you'd copy to .vscode in order to use and maybe customize further – any changes to the example would have to be deliberate by design (except if you use symlinks, but then that's your own fault)

@deen13
Copy link
Collaborator Author

deen13 commented Apr 23, 2024

I think it's unlikely for someone to change the file by accident because changes made in Visual Studio Code are not written to the settings.json. I agree with you that it feels weird to commit dotfiles, but in this case, it adds value to the developer experience and onboarding. Let me know if I've overlooked any cases where having the file would be problematic. 🙈

@deen13
Copy link
Collaborator Author

deen13 commented Apr 23, 2024

I have an idea for a tradeoff. How about removing it from here and adding it to the devcontainer configuration, as it already contains some ruff related stuff? @PeterNerlich @charludo

@deen13 deen13 changed the title Configure ruff import sorting and default settings for code Configure ruff import sorting Apr 23, 2024
@charludo
Copy link
Contributor

I have an idea for a tradeoff. How about removing it from here and adding it to the devcontainer configuration, as it already contains some rough-related stuff? @PeterNerlich @charludo

Sounds like a good idea to me!!

@deen13 deen13 changed the title Configure ruff import sorting Configure ruff import sorting and drop isort support Apr 26, 2024
Copy link
Contributor

@charludo charludo left a comment

Choose a reason for hiding this comment

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

LGTM, apart from one tiny thing

@@ -124,13 +124,13 @@ with :github-source:`tools/integreat-cms-cli`::
Code Quality
============

Automatically sort python import statements with :github-source:`tools/isort.sh`::
(Deprecated) Automatically apply our python style with :github-source:`tools/black.sh`::
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
(Deprecated) Automatically apply our python style with :github-source:`tools/black.sh`::
Automatically apply our python style with :github-source:`tools/black.sh`::

I'd argue it is not deprecated yet, since using ruff format leads to a very lage diff right now.

Copy link
Contributor

@cclauss cclauss Apr 29, 2024

Choose a reason for hiding this comment

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

You might want to consider ruff format as a highly compatible but much faster replacement for black.

The initial diff might be large but is often about whitespace and is a one-time change.

Copy link
Contributor

Choose a reason for hiding this comment

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

The diff is largely due to currently wrapped lines (black's default line length is 88) being put on a single line (pyproject.toml configures line lengths of 249 for ruff, apparently due to differences in the way comment strings are habndled, at least so I've been told)

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

From that article:

We agree that Ruff's formatting (that matches Black's 23) is hard to read and needs improvement. But we aren't convinced that parenthesizing long nested expressions is the best solution, especially when considering expression formatting holistically. That's why we want to defer the decision until we've explored alternative nested expression formatting styles.

However, setting

[tool.ruff]
line-length = 88

indeed does introduce only a handful of changes - @timobrembeck, did I misunderstand the reason for setting it to 279? Because just from this it looks like we could indeed shorten this.

Copy link
Contributor

@cclauss cclauss May 7, 2024

Choose a reason for hiding this comment

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

Use ruff check --select=I instead of isort and use ruff format instead of black? Highly compatible and much faster.

Copy link
Contributor

Choose a reason for hiding this comment

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

+1 for ruff format @timobrembeck that throws 0 errors and only reformats 15 files

Copy link
Contributor

@cclauss cclauss May 7, 2024

Choose a reason for hiding this comment

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

Awesome... Use ruff check --select=PL instead of pylint? Highly compatible and much faster.

Copy link
Collaborator Author

@deen13 deen13 May 8, 2024

Choose a reason for hiding this comment

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

@cclauss, this seems like a very good idea to me in general, but Ruff does not yet support all pylint rules. Therefore, I'd prefer to wait a bit and ensure a smooth migration later on.

Progress on pylint rules: astral-sh/ruff#970

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@charludo ruff formatting does work on my local setup as well, but it fails within the pre-commit hook and on circleci as well. I guess we have to wait with the formatter migration until the bug in the rule is fixed.

@cclauss
Copy link
Contributor

cclauss commented Apr 29, 2024

Are you recommending the use of Ruff extension for Visual Studio Code?

@deen13
Copy link
Collaborator Author

deen13 commented Apr 30, 2024

Are you recommending the use of Ruff extension for Visual Studio Code?

@cclauss yep it is predefined in our setup for code users

@@ -124,11 +124,11 @@ with :github-source:`tools/integreat-cms-cli`::
Code Quality
============

(Deprecated) Automatically apply our python style with :github-source:`tools/black.sh`::
Automatically apply our python style with :github-source:`tools/black.sh`::
Copy link
Contributor

@cclauss cclauss May 8, 2024

Choose a reason for hiding this comment

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

Suggested change
Automatically apply our python style with :github-source:`tools/black.sh`::
Automatically apply our Python style with :github-source:`tools/black.sh`::


./tools/black.sh

Automatically apply our python style with :github-source:`tools/ruff.sh`::
Automatically run python linting with :github-source:`tools/ruff.sh`::
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Automatically run python linting with :github-source:`tools/ruff.sh`::
Automatically run Python linting with :github-source:`tools/ruff.sh`::

Copy link
Contributor

@charludo charludo left a comment

Choose a reason for hiding this comment

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

👍🏼

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.

None yet

5 participants