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

Usage of extend-unsafe-fixes = ["ALL"] option #8404

Closed
lukaspiatkowski opened this issue Nov 1, 2023 · 3 comments · Fixed by #8444
Closed

Usage of extend-unsafe-fixes = ["ALL"] option #8404

lukaspiatkowski opened this issue Nov 1, 2023 · 3 comments · Fixed by #8444
Assignees
Labels
configuration Related to settings and configuration

Comments

@lukaspiatkowski
Copy link
Contributor

This is more of a feature request than a bug. I would like to configure Ruff in a manner that I can apply different set of fixes depending on the context when it is invoked.
Some fixes that ruff has are considered to be "safe" since they don't change the semantic of the code, but they might indicate a typo or a mistake done by the author of code, e.g. the B033 - Sets should not contain duplicate item {value} might just mean that the author mistyped the value they wanted to the set. For those set of rules I would like them to require a manual call to ruff. Lets call them "manual" fixes for now.
Other are quite straightforward (like I001 - Import block is un-sorted or un-formatted), those I would like to be applied by a pre-commit hook automatically even without code authors knowledge. Lets call them "automatic" fixes for now.
An additional feature would be for the Ruff integration with IDE work as expected (at least the VSCode one) - when format-on-save action is invoked the "automatic" rules are applied, but not the "manual" ones. It would be nice though to be still able to fix the "manual" ones with a Quick Fix action.

There are two solutions I was thinking of how I could achieve this:

  • Use fixable/extend-fixable configs to set it to include only the "automatic" fixes. In order to call the "manual" fixes ruff will have to be called with --fixable ALL. The disadvantage of this is that IDE doesn't pick up the Quick Fix action for "manual" fixes as they are unfixable.
  • Use extend-safe-fixes/extend-unsafe-fixes to mark all "automatic" fixes as safe and "manual" fixes as unsafe. In order to call the "manual" fixes ruff will have to be called with --unsafe-fixes and the IDE integration works as expected. The problem with this approach is that it isn't easy to maintain the config. It would be ideal to write:
extend-unsafe-fixes = ["ALL"]
extend-safe-fixes = [
  "I001", # isort
  "F401", # remove unused imports
  "UP",   # pyupgrade
]

But this doesn't work on 0.1.3 - all fixes are marked as unsafe and the extend-safe-fixes is essentially ignored.

Could you please either reverse the precedence of those configs or make it smarter? E.g. more specialized "UP" overrides "ALL" and "F401" overrides "F"? It is unclear to me though how this would work with the extend = "<other config>" option that we are also a heavy user of.

@lukaspiatkowski
Copy link
Contributor Author

lukaspiatkowski commented Nov 1, 2023

Another suggestion would be to add a safe-fixes config that when unset would default to the current behavior, but if set to safe-fixes = ["ALL"] or safe-fixes = [ ] would change the base to be all fixes are safe or no fixes are safe respectively. Then apply the extend-* configs on top of that. This matches the select and extend-select behavior.

@zanieb
Copy link
Member

zanieb commented Nov 1, 2023

Hm.. extend-unsafe-fixes takes precedence over extend-safe-fixes because we would rather err on the side of caution if someone uses both. We can update that to take into account the specificity of the selectors you've provided though, that's how other configuration options work.

Are you interested in contributing? Otherwise I can take a look at it.

re safe-fixes I'd prefer not to add another configuration option yet.

@lukaspiatkowski
Copy link
Contributor Author

Sorry for not giving you an answer straight away @zanieb, I wasn't sure if I will have time to write the PR, but in the end I did.

zanieb added a commit that referenced this issue Nov 7, 2023
…nd `override extend_safe_fixes` (#8444)

## Summary

Prior to this change `extend_unsafe_fixes` took precedence over
`extend_safe_fixes` selectors, so any conflicts were resolved in favour
of `extend_unsafe_fixes`. Thanks to that ruff were conservatively
assuming that if configs conlict the fix corresponding to selected rule
will be treated as unsafe.

After this change we take into account Specificity of the selectors. For
conflicts between selectors of the same Specificity we will treat the
corresponding fixes as unsafe. But if the conflicting selectors are of
different specificity the more specific one will win.

## Test Plan

Tests were added for the `FixSafetyTable` struct. The
`check_extend_unsafe_fixes_conflict_with_extend_safe_fixes_by_specificity`
integration test was added to test conflicting rules of different
specificity.

Fixes #8404

---------

Co-authored-by: Zanie <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
configuration Related to settings and configuration
Projects
None yet
2 participants