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

Change CLI to respect Applicability #4185

Closed
Tracked by #4181
MichaReiser opened this issue May 2, 2023 · 4 comments · Fixed by #7769
Closed
Tracked by #4181

Change CLI to respect Applicability #4185

MichaReiser opened this issue May 2, 2023 · 4 comments · Fixed by #7769
Assignees
Labels
breaking Breaking API change cli Related to the command-line interface help wanted Contributions especially welcome

Comments

@MichaReiser
Copy link
Member

MichaReiser commented May 2, 2023

This task is part of #4181 and depends on #4183. The goal is to change the CLI to respect the Fix::applicability.

  • Change --fix to only apply Applicability::Safe fixes. Print a message with the number of fixes that were skipped because they have an Applicability other than Safe and print the number at the end.
  • Add a new --fix-unsafe CLI option that applies all fixes
  • Change --diff and --fix-only to imply --fix but runs all fixes if the user also provides --fix-unsafe.

Considerations

  • We may want to hold off with this change until we've categorized a significant part of the fixes (see Categorise fixes as safe or maybe_incorrect #4184).
  • I recommend introducing a new --fix-unsafe to apply unsafe fixes of past experience at Rome. Rome used to apply all fixes with --fix but we got multiple reports from users that they did not expect Rome to make changes that may change the program's semantic. That's why we decided to rename the CLI option to --apply and --apply-unsafe to make it clear, that the latter applies fixes that may be incorrect. The downside of not running all fixes with --fix is that I expect that some users may be confused why ruff doesn't apply all fixes when running --fix.
@MichaReiser MichaReiser added breaking Breaking API change help wanted Contributions especially welcome cli Related to the command-line interface labels May 2, 2023
@charliermarsh
Copy link
Member

Are Applicability::Manual fixes included with --fix-unsafe?

@MichaReiser
Copy link
Member Author

Are Applicability::Manual fixes included with --fix-unsafe?

No. Manual fixes are never applied automatically. Similar to clippy when it tells you to use a if let chain instead of match

@evanrittenhouse
Copy link
Contributor

evanrittenhouse commented Jun 14, 2023

You can assign this to me - seems like there have been several rules that have already been categorized, so there's at least a good sample size to test with. :)

@MichaReiser
Copy link
Member Author

design discussion: #5476

zanieb added a commit that referenced this issue Oct 6, 2023
Rebase of #5119 authored by
@evanrittenhouse with additional refinements.

## Changes

- Adds `--unsafe-fixes` / `--no-unsafe-fixes` flags to `ruff check`
- Violations with unsafe fixes are not shown as fixable unless opted-in
- Fix applicability is respected now
    - `Applicability::Never` fixes are no longer applied
    - `Applicability::Sometimes` fixes require opt-in
    - `Applicability::Always` fixes are unchanged
- Hints for availability of `--unsafe-fixes` added to `ruff check`
output

## Examples

Check hints at hidden unsafe fixes
```
❯ ruff check example.py --no-cache --select F601,W292
example.py:1:14: F601 Dictionary key literal `'a'` repeated
example.py:2:15: W292 [*] No newline at end of file
Found 2 errors.
[*] 1 fixable with the `--fix` option (1 hidden fix can be enabled with the `--unsafe-fixes` option).
```

We could add an indicator for which violations have hidden fixes in the
future.

Check treats unsafe fixes as applicable with opt-in
```
❯ ruff check example.py --no-cache --select F601,W292 --unsafe-fixes
example.py:1:14: F601 [*] Dictionary key literal `'a'` repeated
example.py:2:15: W292 [*] No newline at end of file
Found 2 errors.
[*] 2 fixable with the --fix option.
```

Also can be enabled in the config file

```
❯ cat ruff.toml
unsafe-fixes = true
```

And opted-out per invocation

```
❯ ruff check example.py --no-cache --select F601,W292 --no-unsafe-fixes
example.py:1:14: F601 Dictionary key literal `'a'` repeated
example.py:2:15: W292 [*] No newline at end of file
Found 2 errors.
[*] 1 fixable with the `--fix` option (1 hidden fix can be enabled with the `--unsafe-fixes` option).
```

Diff does not include unsafe fixes
```
❯ ruff check example.py --no-cache --select F601,W292 --diff
--- example.py
+++ example.py
@@ -1,2 +1,2 @@
 x = {'a': 1, 'a': 1}
-print(('foo'))
+print(('foo'))
\ No newline at end of file

Would fix 1 error.
```

Unless there is opt-in
```
❯ ruff check example.py --no-cache --select F601,W292 --diff --unsafe-fixes
--- example.py
+++ example.py
@@ -1,2 +1,2 @@
-x = {'a': 1}
-print(('foo'))
+x = {'a': 1, 'a': 1}
+print(('foo'))
\ No newline at end of file

Would fix 2 errors.
```

#7790 will improve the diff
messages following this pull request

Similarly, `--fix` and `--fix-only` require the `--unsafe-fixes` flag to
apply unsafe fixes.

## Related

Replaces #5119
Closes #4185
Closes #7214
Closes #4845
Closes #3863
Addresses #6835
Addresses #7019
Needs follow-up #6962
Needs follow-up #4845
Needs follow-up #7436
Needs follow-up #7025
Needs follow-up #6434
Follow-up #7790 
Follow-up #7792

---------

Co-authored-by: Evan Rittenhouse <[email protected]>
konstin pushed a commit that referenced this issue Oct 11, 2023
Rebase of #5119 authored by
@evanrittenhouse with additional refinements.

## Changes

- Adds `--unsafe-fixes` / `--no-unsafe-fixes` flags to `ruff check`
- Violations with unsafe fixes are not shown as fixable unless opted-in
- Fix applicability is respected now
    - `Applicability::Never` fixes are no longer applied
    - `Applicability::Sometimes` fixes require opt-in
    - `Applicability::Always` fixes are unchanged
- Hints for availability of `--unsafe-fixes` added to `ruff check`
output

## Examples

Check hints at hidden unsafe fixes
```
❯ ruff check example.py --no-cache --select F601,W292
example.py:1:14: F601 Dictionary key literal `'a'` repeated
example.py:2:15: W292 [*] No newline at end of file
Found 2 errors.
[*] 1 fixable with the `--fix` option (1 hidden fix can be enabled with the `--unsafe-fixes` option).
```

We could add an indicator for which violations have hidden fixes in the
future.

Check treats unsafe fixes as applicable with opt-in
```
❯ ruff check example.py --no-cache --select F601,W292 --unsafe-fixes
example.py:1:14: F601 [*] Dictionary key literal `'a'` repeated
example.py:2:15: W292 [*] No newline at end of file
Found 2 errors.
[*] 2 fixable with the --fix option.
```

Also can be enabled in the config file

```
❯ cat ruff.toml
unsafe-fixes = true
```

And opted-out per invocation

```
❯ ruff check example.py --no-cache --select F601,W292 --no-unsafe-fixes
example.py:1:14: F601 Dictionary key literal `'a'` repeated
example.py:2:15: W292 [*] No newline at end of file
Found 2 errors.
[*] 1 fixable with the `--fix` option (1 hidden fix can be enabled with the `--unsafe-fixes` option).
```

Diff does not include unsafe fixes
```
❯ ruff check example.py --no-cache --select F601,W292 --diff
--- example.py
+++ example.py
@@ -1,2 +1,2 @@
 x = {'a': 1, 'a': 1}
-print(('foo'))
+print(('foo'))
\ No newline at end of file

Would fix 1 error.
```

Unless there is opt-in
```
❯ ruff check example.py --no-cache --select F601,W292 --diff --unsafe-fixes
--- example.py
+++ example.py
@@ -1,2 +1,2 @@
-x = {'a': 1}
-print(('foo'))
+x = {'a': 1, 'a': 1}
+print(('foo'))
\ No newline at end of file

Would fix 2 errors.
```

#7790 will improve the diff
messages following this pull request

Similarly, `--fix` and `--fix-only` require the `--unsafe-fixes` flag to
apply unsafe fixes.

## Related

Replaces #5119
Closes #4185
Closes #7214
Closes #4845
Closes #3863
Addresses #6835
Addresses #7019
Needs follow-up #6962
Needs follow-up #4845
Needs follow-up #7436
Needs follow-up #7025
Needs follow-up #6434
Follow-up #7790 
Follow-up #7792

---------

Co-authored-by: Evan Rittenhouse <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking Breaking API change cli Related to the command-line interface help wanted Contributions especially welcome
Projects
None yet
3 participants