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

COM818 with ruff format fixes the error too harshly #14427

Open
Neikow opened this issue Nov 18, 2024 · 2 comments
Open

COM818 with ruff format fixes the error too harshly #14427

Neikow opened this issue Nov 18, 2024 · 2 comments
Labels
incompatibility Incompatibility between different Ruff tools or rules (formatter, isort, linter)

Comments

@Neikow
Copy link

Neikow commented Nov 18, 2024

Hi ! Thanks for providing us with such a great tool !

When the rule COM818 is turned on and we run the formatter, it will wrap the trailing comma with paranthesis so when we run the linter again, the error will be corrected although it was probably a mistake to begin with.

Playground URL

I think it would be better to prevent the formatter to run on these types of issues when this particular rule is selected as it can lead to hard to spot errors.

  • Commands: ruff format / ruff lint
  • Version: v0.7.4
  • Relevant settings:
{
  "lint":
    "select": [
       "COM818"
    ]
}
@MichaReiser
Copy link
Member

As note for myself:

The input is

test = {"ere": ""},

which gets formatted to

test = ({"ere": ""},)

Which is no longer flagged by COM818.

Can you tell me more about your workflow? Our general recommendation is to run the linter (and apply fixes) first, then run the formatter. That the formatter wraps bare tuples in parentheses was also a deliberate decision to make them more visible in, e.g., the editor's case. It also helped to identify many unwanted bare tuples in existing code bases, e.g., Django. But I don't think we ever received feedback on this. So thanks for reporting this specific case where I can see that it can lead to missed possibly-bare tuples.

@MichaReiser MichaReiser added the incompatibility Incompatibility between different Ruff tools or rules (formatter, isort, linter) label Nov 18, 2024
@Neikow
Copy link
Author

Neikow commented Nov 19, 2024

We run ruff via pre-commit with the lint phase before the formatting phase.

  1. When we try to commit the first time, it fails because of a lint error and because the formatter made somme changes.
  2. Usually developer think it was only the formatting that made the hook fail (and might not see the lint error) so they stage files and try to commit again and it pass this time (because de lint error is no longer present)

Arguably the developer should inspect more precisely the output of the first try but in more normal cases, the lint error would still be present after the second try and therefore will not be missed.

We thought about adding a hook for only the COM818 rule which would fail fast but it would increase the runtime of the hooks.
Furthermore, marking ruff lint as fail_fast would mean we would need to commit three times at least:

  • once for the linting
  • once for the formatting
  • accepted commit

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
incompatibility Incompatibility between different Ruff tools or rules (formatter, isort, linter)
Projects
None yet
Development

No branches or pull requests

2 participants