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.

Sign up for GitHub

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

Display fixes by default (#7352) #12595

Closed
wants to merge 4 commits into from
Closed

Display fixes by default (#7352) #12595

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
b824fd3
Display fixes by default (#7352)
chriskrycho Jul 31, 2024
1f07f5f
Remove 'Suggested fix:' header from `check` diff output
chriskrycho Jul 31, 2024
8fabf4f
Only show safe fixes in `check` output for now
chriskrycho Jul 31, 2024
fde34bc
Show only diffs, not CTAs, in `check`
chriskrycho Jul 31, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
The table of contents is too big for display.
Diff view
Diff view
  •  
  •  
  •  
3 changes: 3 additions & 0 deletions crates/ruff/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -287,6 +287,9 @@ pub fn check(args: CheckCommand, global_options: GlobalConfigArgs) -> Result<Exi
if show_fixes {
printer_flags |= PrinterFlags::SHOW_FIX_SUMMARY;
}
if output_format == OutputFormat::Full {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be gated behind preview mode to get some user feedback before enabling it for everyone?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think it's strictly necessary, but we may want to do so if it's going to change significantly (i.e. by being split into follow-ups)

printer_flags |= PrinterFlags::SHOW_FIX_DIFF;
}

#[cfg(debug_assertions)]
if cache {
Expand Down
187 changes: 187 additions & 0 deletions crates/ruff/tests/integration_test.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -122,6 +122,11 @@ fn stdin_error() {
|
= help: Remove unused import: `os`

Suggested fix:
1 |-import os

Run `ruff check --fix` to apply this fix.

Found 1 error.
[*] 1 fixable with the `--fix` option.

Expand All @@ -146,6 +151,11 @@ fn stdin_filename() {
|
= help: Remove unused import: `os`

Suggested fix:
1 |-import os

Run `ruff check --fix` to apply this fix.

Found 1 error.
[*] 1 fixable with the `--fix` option.

Expand Down Expand Up @@ -181,13 +191,25 @@ import bar # unused import
|
= help: Remove unused import: `bar`

Suggested fix:
1 1 |
2 |-import bar # unused import

Run `ruff check --fix` to apply this fix.

foo.py:2:8: F401 [*] `foo` imported but unused
|
2 | import foo # unused import
| ^^^ F401
|
= help: Remove unused import: `foo`

Suggested fix:
1 1 |
2 |-import foo # unused import

Run `ruff check --fix` to apply this fix.

Found 2 errors.
[*] 2 fixable with the `--fix` option.

Expand Down Expand Up @@ -215,6 +237,11 @@ fn check_warn_stdin_filename_with_files() {
|
= help: Remove unused import: `os`

Suggested fix:
1 |-import os

Run `ruff check --fix` to apply this fix.

Found 1 error.
[*] 1 fixable with the `--fix` option.

Expand All @@ -241,6 +268,11 @@ fn stdin_source_type_py() {
|
= help: Remove unused import: `os`

Suggested fix:
1 |-import os

Run `ruff check --fix` to apply this fix.

Found 1 error.
[*] 1 fixable with the `--fix` option.

Expand Down Expand Up @@ -576,13 +608,29 @@ fn stdin_override_parser_ipynb() {
|
= help: Remove unused import: `os`

Suggested fix:
1 |-import os
2 1 | import sys
3 2 | print(1)
4 3 |

Run `ruff check --fix` to apply this fix.

Jupyter.py:cell 3:1:8: F401 [*] `sys` imported but unused
|
1 | import sys
| ^^^ F401
|
= help: Remove unused import: `sys`

Suggested fix:
1 1 | import os
2 |-import sys
3 2 | print(1)
4 3 |

Run `ruff check --fix` to apply this fix.

Found 2 errors.
[*] 2 fixable with the `--fix` option.

Expand Down Expand Up @@ -612,6 +660,11 @@ fn stdin_override_parser_py() {
|
= help: Remove unused import: `os`

Suggested fix:
1 |-import os

Run `ruff check --fix` to apply this fix.

Found 1 error.
[*] 1 fixable with the `--fix` option.

Expand Down Expand Up @@ -1502,6 +1555,11 @@ fn check_input_from_argfile() -> Result<()> {
|
= help: Remove unused import: `os`

Suggested fix:
chriskrycho marked this conversation as resolved.
Show resolved Hide resolved
1 |-import os

Run `ruff check --fix` to apply this fix.

Found 1 error.
[*] 1 fixable with the `--fix` option.

Expand All @@ -1523,7 +1581,17 @@ fn check_hints_hidden_unsafe_fixes() {
exit_code: 1
----- stdout -----
-:1:1: RUF901 [*] Hey this is a stable test rule with a safe fix.
Suggested fix:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like we need to do something special for violations without help messages? We want a dedicated test case for that and a test case for violations without source context.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I missed that one. I’ll see about adding a test for those. What’s a scenario where you would end up without source context?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, for example, there's no source context for this test rule right? I think if the violation is attached to an empty range we elide the source context. This behavior was recently modified in #12304

Copy link
Contributor Author

@chriskrycho chriskrycho Jul 31, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm going to open a new issue tracking what to do in each of those cases, with the existing baseline implementation here as a useful starting point.

1 |+# fix from stable-test-rule-safe-fix

Run `ruff check --fix` to apply this fix.
Copy link
Member

@zanieb zanieb Jul 31, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking at this more holistically, I wonder if this message is necessary since we have a call to action at the end:

[*] 1 fixable with the --fix option (1 hidden fix can be enabled with the --unsafe-fixes option).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I saw that as well, but my intuition was that it is helpful to have both the details and the summary.


-:1:1: RUF902 Hey this is a stable test rule with an unsafe fix.
Suggested fix:
1 |+# fix from stable-test-rule-unsafe-fix

Run `ruff check --fix --unsafe-fixes` to apply this unsafe fix.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given the message below this ("1 hidden fix..."), I wonder if we should be hiding unsafe fixes unless the opt-in is provided. I think this would solve the "how do we display applicability" problem, to an extent?

If so, I think we could figure out what to do with "display-only" fixes later and just never show them to start (to limit the scope of this pull request)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would require a bit of futzing to retain the existing coverage in our test suite, maybe just using --unsafe-fixes by default in some places?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, yeah—I can definitely back out those parts of the change if that’s preferable!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It feels like the current design (prior to this pull request) hides unsafe fixes and allowing opt-in to their display, at which point we don't need to indicate that they're unsafe? It seems easiest to follow that pattern for now and consider if and how we want to display unsafe fixes by default separately, i.e., I think more discussion in an issue would be prudent.

Copy link
Contributor Author

@chriskrycho chriskrycho Jul 31, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In terms of handling applicability, we could do that in a couple of ways:

  • The Diff itself could take a configuration level and its Display implementation could use that to decide what to print.
  • The caller could decide whether to print based on message applicability at a higher level, keeping it out of the Diff.

The latter is a slightly smaller scope and IMO better isolation of concerns; it keeps it properly in the TextEmitter, whereas the Diff can be concerned entirely with the presentation of a given diff.

If we go that way, we still have the choice whether to keep the applicability message in the diff. I think I am still mildly inclined toward showing that CTA, because of the differences when you do include --unsafe-fixes; it seems like it would still be nice to know which changes are which.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems reasonable to decide whether or not the Diff should be displayed outside its display implementation (the latter option).

Regarding the CTA, I have a preference for splitting it out of this pull request so we can merge this sooner and have focused discussion on each objective, like:

  1. Display available fixes by default
  2. Disambiguate safe and unsafe fixes when displayed (i.e., via a CTA or other label)
  3. Display unsafe fixes by default (needs discussion)
  4. Determine how to make display-only fixes user-facing

What do you think?

Copy link
Contributor Author

@chriskrycho chriskrycho Jul 31, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That makes good sense to me. I will split out issues (Edit: #12598) for further discussion on those points and update this to simply display fixes only for the Applicability::Safe level, in a slightly clunky way that is amenable to cleaning up by way of broader refactors once some of those other design questions are fleshed out.


Found 2 errors.
[*] 1 fixable with the `--fix` option (1 hidden fix can be enabled with the `--unsafe-fixes` option).

Expand All @@ -1541,6 +1609,12 @@ fn check_hints_hidden_unsafe_fixes_with_no_safe_fixes() {
exit_code: 1
----- stdout -----
-:1:1: RUF902 Hey this is a stable test rule with an unsafe fix.
Suggested fix:
1 |+# fix from stable-test-rule-unsafe-fix
1 2 | x = {'a': 1, 'a': 1}

Run `ruff check --fix --unsafe-fixes` to apply this unsafe fix.

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

Expand All @@ -1559,7 +1633,17 @@ fn check_no_hint_for_hidden_unsafe_fixes_when_disabled() {
exit_code: 1
----- stdout -----
-:1:1: RUF901 [*] Hey this is a stable test rule with a safe fix.
Suggested fix:
1 |+# fix from stable-test-rule-safe-fix

Run `ruff check --fix` to apply this fix.

-:1:1: RUF902 Hey this is a stable test rule with an unsafe fix.
Suggested fix:
1 |+# fix from stable-test-rule-unsafe-fix

Run `ruff check --fix --unsafe-fixes` to apply this unsafe fix.

Found 2 errors.
[*] 1 fixable with the --fix option.

Expand All @@ -1579,6 +1663,12 @@ fn check_no_hint_for_hidden_unsafe_fixes_with_no_safe_fixes_when_disabled() {
exit_code: 1
----- stdout -----
-:1:1: RUF902 Hey this is a stable test rule with an unsafe fix.
Suggested fix:
1 |+# fix from stable-test-rule-unsafe-fix
1 2 | x = {'a': 1, 'a': 1}

Run `ruff check --fix --unsafe-fixes` to apply this unsafe fix.

Found 1 error.

----- stderr -----
Expand All @@ -1596,7 +1686,17 @@ fn check_shows_unsafe_fixes_with_opt_in() {
exit_code: 1
----- stdout -----
-:1:1: RUF901 [*] Hey this is a stable test rule with a safe fix.
Suggested fix:
1 |+# fix from stable-test-rule-safe-fix

Run `ruff check --fix` to apply this fix.

-:1:1: RUF902 [*] Hey this is a stable test rule with an unsafe fix.
Suggested fix:
1 |+# fix from stable-test-rule-unsafe-fix

Run `ruff check --fix --unsafe-fixes` to apply this unsafe fix.

Found 2 errors.
[*] 2 fixable with the --fix option.

Expand All @@ -1618,6 +1718,12 @@ fn fix_applies_safe_fixes_by_default() {

----- stderr -----
-:1:1: RUF902 Hey this is a stable test rule with an unsafe fix.
Suggested fix:
1 |+# fix from stable-test-rule-unsafe-fix
1 2 | # fix from stable-test-rule-safe-fix

Run `ruff check --fix --unsafe-fixes` to apply this unsafe fix.

Found 2 errors (1 fixed, 1 remaining).
No fixes available (1 hidden fix can be enabled with the `--unsafe-fixes` option).
"###);
Expand Down Expand Up @@ -1655,6 +1761,12 @@ fn fix_does_not_apply_display_only_fixes() {
def add_to_list(item, some_list=[]): ...
----- stderr -----
-:1:1: RUF903 Hey this is a stable test rule with a display only fix.
Suggested fix:
1 |+# fix from stable-test-rule-display-only-fix
1 2 | def add_to_list(item, some_list=[]): ...

Ruff cannot safely apply this fix.

Found 1 error.
"###);
}
Expand All @@ -1673,6 +1785,12 @@ fn fix_does_not_apply_display_only_fixes_with_unsafe_fixes_enabled() {
def add_to_list(item, some_list=[]): ...
----- stderr -----
-:1:1: RUF903 Hey this is a stable test rule with a display only fix.
Suggested fix:
1 |+# fix from stable-test-rule-display-only-fix
1 2 | def add_to_list(item, some_list=[]): ...

Ruff cannot safely apply this fix.

Found 1 error.
"###);
}
Expand All @@ -1690,6 +1808,11 @@ fn fix_only_unsafe_fixes_available() {

----- stderr -----
-:1:1: RUF902 Hey this is a stable test rule with an unsafe fix.
Suggested fix:
1 |+# fix from stable-test-rule-unsafe-fix

Run `ruff check --fix --unsafe-fixes` to apply this unsafe fix.

Found 1 error.
No fixes available (1 hidden fix can be enabled with the `--unsafe-fixes` option).
"###);
Expand Down Expand Up @@ -1826,7 +1949,17 @@ extend-unsafe-fixes = ["RUF901"]
exit_code: 1
----- stdout -----
-:1:1: RUF901 Hey this is a stable test rule with a safe fix.
Suggested fix:
1 |+# fix from stable-test-rule-safe-fix

Run `ruff check --fix --unsafe-fixes` to apply this unsafe fix.

-:1:1: RUF902 Hey this is a stable test rule with an unsafe fix.
Suggested fix:
1 |+# fix from stable-test-rule-unsafe-fix

Run `ruff check --fix --unsafe-fixes` to apply this unsafe fix.

Found 2 errors.
No fixes available (2 hidden fixes can be enabled with the `--unsafe-fixes` option).

Expand Down Expand Up @@ -1858,7 +1991,17 @@ extend-safe-fixes = ["RUF902"]
exit_code: 1
----- stdout -----
-:1:1: RUF901 [*] Hey this is a stable test rule with a safe fix.
Suggested fix:
1 |+# fix from stable-test-rule-safe-fix

Run `ruff check --fix` to apply this fix.

-:1:1: RUF902 [*] Hey this is a stable test rule with an unsafe fix.
Suggested fix:
1 |+# fix from stable-test-rule-unsafe-fix

Run `ruff check --fix` to apply this fix.

Found 2 errors.
[*] 2 fixable with the `--fix` option.

Expand Down Expand Up @@ -1892,7 +2035,17 @@ extend-safe-fixes = ["RUF902"]
exit_code: 1
----- stdout -----
-:1:1: RUF901 [*] Hey this is a stable test rule with a safe fix.
Suggested fix:
1 |+# fix from stable-test-rule-safe-fix

Run `ruff check --fix` to apply this fix.

-:1:1: RUF902 Hey this is a stable test rule with an unsafe fix.
Suggested fix:
1 |+# fix from stable-test-rule-unsafe-fix

Run `ruff check --fix --unsafe-fixes` to apply this unsafe fix.

Found 2 errors.
[*] 1 fixable with the `--fix` option (1 hidden fix can be enabled with the `--unsafe-fixes` option).

Expand Down Expand Up @@ -1929,8 +2082,32 @@ extend-safe-fixes = ["RUF9"]
----- stdout -----
-:1:1: RUF900 Hey this is a stable test rule.
-:1:1: RUF901 Hey this is a stable test rule with a safe fix.
Suggested fix:
1 |+# fix from stable-test-rule-safe-fix
1 2 | x = {'a': 1, 'a': 1}
2 3 | print(('foo'))
3 4 | print(str('foo'))

Run `ruff check --fix --unsafe-fixes` to apply this unsafe fix.

-:1:1: RUF902 [*] Hey this is a stable test rule with an unsafe fix.
Suggested fix:
1 |+# fix from stable-test-rule-unsafe-fix
1 2 | x = {'a': 1, 'a': 1}
2 3 | print(('foo'))
3 4 | print(str('foo'))

Run `ruff check --fix` to apply this fix.

-:1:1: RUF903 Hey this is a stable test rule with a display only fix.
Suggested fix:
1 |+# fix from stable-test-rule-display-only-fix
1 2 | x = {'a': 1, 'a': 1}
2 3 | print(('foo'))
3 4 | print(str('foo'))

Ruff cannot safely apply this fix.

-:1:1: RUF920 Hey this is a deprecated test rule.
-:1:1: RUF921 Hey this is another deprecated test rule.
-:1:1: RUF950 Hey this is a test rule that was redirected from another.
Expand Down Expand Up @@ -2039,6 +2216,16 @@ select = ["RUF017"]
|
= help: Replace with `functools.reduce`

Suggested fix:
1 |+import functools
2 |+import operator
1 3 | x = [1, 2, 3]
2 4 | y = [4, 5, 6]
3 |-sum([x, y], [])
5 |+functools.reduce(operator.iadd, [x, y], [])

Run `ruff check --fix --unsafe-fixes` to apply this unsafe fix.

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

Expand Down
Loading
Loading