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

Apply preview ruff rules #12981

Closed
wants to merge 5 commits into from

Conversation

DimitriPapadopoulos
Copy link
Contributor

@DimitriPapadopoulos DimitriPapadopoulos commented Sep 28, 2024

These issues appear when running ruff check --preview. The rules are not yet applied by default, but should become part of the default rulesets in future versions. The purpose of this PR is to make the code future-proof.

I have not addressed code complexity issues:

There are multiple ways to address such issues;

  1. Refactor the code to reduce complexity, but that might be a bad idea here.
  2. Disable the relevant rules.
  3. Modify the threshold value using the associated options

Finally, note that I cannot silence a residual issue, caused by a ruff bug:

$  ruff check --preview --ignore PLR0904,PLR0914,PLR0916
tests/unit/test_pyproject_config.py:26:9: F841 Local variable `options` is assigned to but never used
   |
24 |     # Invalid argument exits with an error
25 |     with pytest.raises(SystemExit):
26 |         options, _ = i.parse_args(["xxx", "--config-settings", "x"])
   |         ^^^^^^^ F841
   |
   = help: Remove assignment to unused variable `options`

Found 1 error.
No fixes available (1 hidden fix can be enabled with the `--unsafe-fixes` option).
$ 

@DimitriPapadopoulos DimitriPapadopoulos changed the title Apply preview ruff fules Apply preview ruff rules Sep 28, 2024
@notatallshaw
Copy link
Member

It's a subjective preference, but I prefer that unused variables var gets changed to _var instead of _.

There's a few cases where this has been applied, but in lots of cases they are renamed to _. My reasoning for preferring _var is that it's more clear to readers what that variable is supposed to mean, and if it is reused in the future the name is easy to rename back.

Pip maintainers may disagree though.

@DimitriPapadopoulos
Copy link
Contributor Author

I prefer _var instead of _ myself, especially since that's what ruff check --fix --unsafe-fixes does automatically — much easier to fix for me! However, some lines here become too long and get split. That's especially true of lines which unpack into options, args. I'd rather use _ than splitting these lines.

@pradyunsg
Copy link
Member

Let's not do this -- if these are not ready to be stable rules in ruff, I don't think we need to go out of our way and eagerly enable things.

@DimitriPapadopoulos
Copy link
Contributor Author

DimitriPapadopoulos commented Sep 28, 2024

The rules have not been enabled in pyproject.toml. I have only applied them to the current codebase.

Some of the rules should be part of the default in the next minor version, which I would expect to be released in a few months. But yes, it's possible to wait for that next version, no hurry.

@pfmoore
Copy link
Member

pfmoore commented Sep 28, 2024

Agreed, let's not pre-emptively apply these rules.

I'm not actually sure that we should automatically apply any new rules that ruff makes default in the future. We do have the option to explicitly switch them off, and there's some advantage to that (it reduces churn in our codebase). One of the points of automatic style rules is to reduce code churn, so if it's increasing the churn, that seems like it's a bad thing to me. We should apply the same logic (review any new rules and decide if we want them) to a change in ruff's defaults as we do to an explicit change to add a new rule.

@pfmoore
Copy link
Member

pfmoore commented Sep 28, 2024

Pip maintainers may disagree though.

FWIW, my personal view is that this is precisely the sort of situation where I'd prefer to let the writer (and the reviewers) of the code judge for themselves what's the more readable option. So I'd rather we didn't try to make or enforce a "one size fits all" global rule.

And following on from that, I'd argue that we should take an "if it isn't broken, don't fix it" approach to code covered by this rule, and leave it alone.

F841 Local variable is assigned to but never used
E226 Missing whitespace around arithmetic operator
E262  Inline comment should start with `# `
C419 Unnecessary list comprehension
C420 Unnecessary dict comprehension for iterable; use `dict.fromkeys` instead
@DimitriPapadopoulos
Copy link
Contributor Author

DimitriPapadopoulos commented Sep 29, 2024

I think we need to distinguish between formatters and linters, even if the following attempt for definitions shows overlaps:

  • The formatter applies a consistent style that's supposed to improve readability and reduce future churn. Style is not supposed to change much.
  • In addition to style, the linter applies rules that are supposed to improve the code and avoid common pitfalls. For example, the E and F rules can be seen as mostly style issues, but the C4 rules are more about performance.

In this case:

  • E and F rules are about style. I can fully understand you might prefer to disable them. I've changed this PR to disable rules F841 and E226. I'm not happy with the changes induced by F841 myself.
  • C4 rules are mostly about performance. I would agree performance improvements are marginal. The proper way to address performance is to precisely measure performance with tools, and identify and fix bottlenecks. On the other hand, the C4 ruleset is already enforced. Rule C419 makes sense, and the resulting small change improves readability by reducing useless indentation. Not so sure about C420.
  • I've moved UP031 to Disable or apply ruff/pyupgrade rules (UP) #12983.

@DimitriPapadopoulos DimitriPapadopoulos deleted the preview branch October 3, 2024 19:42
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 19, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants