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 all commits
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
60 changes: 60 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,8 @@ fn stdin_error() {
|
= help: Remove unused import: `os`

1 |-import os

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

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

1 |-import os

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

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

1 1 |
2 |-import bar # unused import

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

1 1 |
2 |-import foo # unused import

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

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

1 |-import os

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

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

1 |-import os

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

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

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

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

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

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

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

1 |-import os

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

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

1 |-import os

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

Expand All @@ -1523,7 +1551,10 @@ fn check_hints_hidden_unsafe_fixes() {
exit_code: 1
----- stdout -----
-:1:1: RUF901 [*] Hey this is a stable test rule with a safe fix.
1 |+# fix from stable-test-rule-safe-fix

-:1:1: RUF902 Hey this is a stable test rule with an unsafe fix.

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

Expand All @@ -1541,6 +1572,7 @@ 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.

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

Expand All @@ -1559,7 +1591,10 @@ 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.
1 |+# fix from stable-test-rule-safe-fix

-:1:1: RUF902 Hey this is a stable test rule with an unsafe fix.

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

Expand All @@ -1579,6 +1614,7 @@ 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.

Found 1 error.

----- stderr -----
Expand All @@ -1596,7 +1632,10 @@ 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.
1 |+# fix from stable-test-rule-safe-fix

-:1:1: RUF902 [*] Hey this is a stable test rule with an unsafe fix.

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

Expand All @@ -1618,6 +1657,7 @@ fn fix_applies_safe_fixes_by_default() {

----- stderr -----
-:1:1: RUF902 Hey this is a stable test rule with an 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 +1695,7 @@ 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.

Found 1 error.
"###);
}
Expand All @@ -1673,6 +1714,7 @@ 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.

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

----- stderr -----
-:1:1: RUF902 Hey this is a stable test rule with an 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 +1869,9 @@ extend-unsafe-fixes = ["RUF901"]
exit_code: 1
----- stdout -----
-:1:1: RUF901 Hey this is a stable test rule with a safe fix.

-:1:1: RUF902 Hey this is a stable test rule with an 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 +1903,11 @@ extend-safe-fixes = ["RUF902"]
exit_code: 1
----- stdout -----
-:1:1: RUF901 [*] Hey this is a stable test rule with a safe fix.
1 |+# fix from stable-test-rule-safe-fix

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

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

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

-:1:1: RUF902 Hey this is a stable test rule with an 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 +1981,15 @@ 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.

-:1:1: RUF902 [*] Hey this is a stable test rule with an unsafe 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'))

-:1:1: RUF903 Hey this is a stable test rule with a display only 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 +2098,7 @@ select = ["RUF017"]
|
= help: Replace with `functools.reduce`


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

Expand Down
18 changes: 10 additions & 8 deletions crates/ruff_linter/src/message/diff.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,16 @@ impl<'a> Diff<'a> {

impl Display for Diff<'_> {
fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result {
// TODO: Instead of hard-coding this in, the `TextEmitter` should be
// handling it. See https://github.com/astral-sh/ruff/issues/12597 for
// further discussion.
if matches!(
self.fix.applicability(),
Applicability::Unsafe | Applicability::DisplayOnly
) {
return Ok(());
}
Comment on lines +39 to +47
Copy link
Member

Choose a reason for hiding this comment

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

Does this mean that we won't see unsafe fixes in snapshot tests anymore?

I would find this concerning because it means we loose all test coverage for unsafe fixes (of which there are plenty)

Copy link
Member

Choose a reason for hiding this comment

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

Yes, we need to address this before merging.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, we realized this late in our time working on it yesterday. It may warrant going ahead and doing the bit I highlighted in #12597, etc. I think this was a case where trying to keep the change minimal actually makes it harder to land—this particularly bit of coupling wasn’t obvious to me until the very end when we changed from including all the CTAs to including none of them, and turning off.

I think it probably needs the aforementioned refactor (where the TextEmitter handles what to print or not) plus setting the relevant flags based on both --unsafe-fixes and whether it is testing.


// TODO(dhruvmanila): Add support for Notebook cells once it's user-facing
let mut output = String::with_capacity(self.source_code.source_text().len());
let mut last_end = TextSize::default();
Expand All @@ -53,14 +63,6 @@ impl Display for Diff<'_> {

let diff = TextDiff::from_lines(self.source_code.source_text(), &output);

let message = match self.fix.applicability() {
// TODO(zanieb): Adjust this messaging once it's user-facing
Applicability::Safe => "Safe fix",
Applicability::Unsafe => "Unsafe fix",
Applicability::DisplayOnly => "Display-only fix",
};
writeln!(f, "ℹ {}", message.blue())?;

let (largest_old, largest_new) = diff
.ops()
.last()
Expand Down
Loading
Loading