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

Use ruff to also sort imports #3237

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

Matiiss
Copy link
Member

@Matiiss Matiiss commented Nov 24, 2024

Just a capability that was not being utilized (it was done before when we were using black + isort)

From ruff docs:

Sorting imports

Currently, the Ruff formatter does not sort imports. In order to both sort imports and format, call the Ruff linter and then the formatter:

ruff check --select I --fix
ruff format

A unified command for both linting and formatting is planned.

@Matiiss Matiiss requested a review from a team as a code owner November 24, 2024 16:47
@@ -96,6 +96,9 @@ exclude = [
"setup.py",
]

[tool.ruff.lint]
fixable = ["I"]
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this is necessary for the pre-commit. It should probably be select = ["I"]

Copy link
Member Author

Choose a reason for hiding this comment

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

This will be necessary later when further configuring ruff as the linter

Copy link
Contributor

Choose a reason for hiding this comment

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

Okay. Could you elaborate more?

  • reason for using fixable and not select
  • reason it is necessary to include fixable = ["I"]

Copy link
Member Author

Choose a reason for hiding this comment

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

As explained in my other comment, this is not where we should configure linting rules using select because we may want to add other rules on top and then it would lint those rules during the pre-commit run, which we don't want because it is meant purely for formatting the code. So, we override the entire select for formatting using the CLI so that it only cares about that rule and also only fixes that rule. The fixable config won't have any impact on the pre-commit runs in this configuration.
It's not strictly necessary to include the fixable config, but it might help if someone runs ruff check --fix because it won't create a massive diff that they did not want to cause with that command. It's a guard rail in a sense.

Copy link
Contributor

Choose a reason for hiding this comment

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

If select has no impact on this pre-commit run because of the --select option, then we should include select.

With your current changes, the ruff select list implicitly is this (the ruff default, as we do not set it):

select = ["E4", "E7", "E9", "F"]

When the user uses ruff check and ruff check --fix, we only want it to warn them about the rules that we use (in this case, only isort), so that they may fix it before the pre-commit fixes it (including for isort 'formatting').
Rather than restricting just what is fixed, we should restrict what is checked.
What is fixed is always a subset of what is checked, so it is erroneous to include isort in fixable without including it in select. With the current changes, ruff check [--fix] will not detect or fix import sorting because the rule is not selected.

Suggested change
fixable = ["I"]
select = ["I"]

Ruff considers isort to be linting, so we must configure linting rules even if we consider it formatting.

Copy link
Member Author

Choose a reason for hiding this comment

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

Fair enough, I get what you're saying, if a contributor runs ruff check themselves, this would check those default rules and if they did run ruff check --fix, it would not fix anything at all (which is certainly not the expected behavior, since sorting the imports is part of what is expected).
I'm not going to remove the fixable = ["I"] configuration though, I don't see the need to do that and again, we're likely to move to ruff as the linter, so, I would like to avoid the case that it gets released without this fixable rule restriction and fixes rules that contributors may not expect it to fix.

Copy link
Contributor

Choose a reason for hiding this comment

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

I see your reasoning with fixable, but I slightly disagree with it, drawing from my experience using ruff. If we introduce more linting rules in the future, then it means they are enforced within the pygame-ce source, and contributors should be able to fix it.

fixes rules that contributors may not expect it to fix

A contributor would actually expect basically all auto-fixable rules to be fixed, the default. This generally doesn't break anything either, ruff is very careful with auto-fixing.

The intended purpose of fixable and unfixable is to allow the user to manually correct certain rules in case the auto-fix for it may break something. (Of course, many rules in ruff don't support auto-fix in the first place.)
Considering this, we would instead use the more common unfixable list instead of fixable list, so exceptions are for the excluded instead of the included. (The default for fixable is "ALL", then we narrow it down.)

pyproject.toml Show resolved Hide resolved
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