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

Support suggesting different fixes for one violation #2713

Open
not-my-profile opened this issue Feb 10, 2023 · 7 comments
Open

Support suggesting different fixes for one violation #2713

not-my-profile opened this issue Feb 10, 2023 · 7 comments
Labels
fixes Related to suggested fixes for violations

Comments

@not-my-profile
Copy link
Contributor

Some violations can be fixed in different ways. The Language Server Protocol supports multiple code actions for one location, so it would be nice if ruff could also suggest multiple autofixes for one violation.

Examples:

  • try: ...; except ...: pass -> flake8-simplify suggests using contextlib.suppress instead, whereas flake8-bandit suggests logging the exception

Can you think of other examples?

@charliermarsh charliermarsh added the fixes Related to suggested fixes for violations label Feb 10, 2023
@ngnpope
Copy link
Contributor

ngnpope commented Feb 10, 2023

It seems that we could add categories/tags related to autofixing?

  • @safe: Fixes that can be safely applied in 100% of cases.

  • @unsafe: Fixes that may produce unexpected results and need manual checking.

  • @unfixable: Violations cannot be fixed automatically. (Always require manual intervention.)

  • @opinionated: Fixes that may not be the best solution to fixing the violation.

    So, using the case in the description above, we could automatically fix to use contextlib.suppress(...), but we couldn't automatically fix to add logging as we wouldn't be able to provide the context for the log message. In many cases it's better to make sure that logging is not required and wasn't merely forgotten during development.

    I'm not sure if it would ever make sense to have multiple ways of fixing the same issue? How would that be configured? This example is a good case of an automatic vs a manual alternative. Could we ever have more than one automatic fix?

Maybe a fix tagged as @unsafe and @opinionated would require both tags to be enabled to be used (or explicitly enabled by the rule name). That way there may be @safe or @unsafe fixes that are @opinionated. (Obviously the first three defined above are mutually exclusive.)

Was also wondering whether we want to have a tag that implies "sometimes" to capture issues like that raised for SIM108 in #2621 where the autofix is not available in 100% of cases?

@not-my-profile
Copy link
Contributor Author

Yes such a categorization is indeed a good idea, we already have #1997 for tracking that :)

@sbrugman
Copy link
Contributor

Pandas-vet has one rule where multiple fixes are possible: .ix is deprecated; use more explicit .loc or .iloc

@sbrugman
Copy link
Contributor

Another example:

B905 zip-without-explicit-strict has two possible autofixes (strict=True and strict=False)

@JonathanPlasse
Copy link
Contributor

JonathanPlasse commented Feb 18, 2023

Pandas-vet has one rule where multiple fixes are possible: .ix is deprecated; use more explicit .loc or .iloc

B905 zip-without-explicit-strict has two possible autofixes (strict=True and strict=False)

These auto-fixes could be configured in settings.

@sbrugman
Copy link
Contributor

It depends. In most cases the autofix is constant per repository (in which case the autofix can be configured on repo level). We can also imagine to review on a case-by-case basis to be able to decide (something like autobot review would be nice).

@charliermarsh
Copy link
Member

Autobot reference in Ruff?? First time!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fixes Related to suggested fixes for violations
Projects
None yet
Development

No branches or pull requests

5 participants