Skip to content

Commit 706d87f

Browse files
dylwil3dylwil3
authored
Show errors for attempted fixes only when passed --verbose (#15237)
The default logging level for diagnostics includes logs written using the `log` crate with level `error`, `warn`, and `info`. An unsuccessful fix attached to a diagnostic via `try_set_fix` or `try_set_optional_fix` was logged at level `error`. Note that the user would see these messages even without passing `--fix`, and possibly also on lines with `noqa` comments. This PR changes the logging level here to a `debug`. We also found ad-hoc instances of error logging in the implementations of several rules, and have replaced those with either a `debug` or call to `try_set{_optional}_fix`. Closes #15229
1 parent 0837cdd commit 706d87f

File tree

6 files changed

+132
-56
lines changed

6 files changed

+132
-56
lines changed

‎crates/ruff/tests/integration_test.rs

Lines changed: 64 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2126,3 +2126,67 @@ unfixable = ["RUF"]
21262126

21272127
Ok(())
21282128
}
2129+
2130+
#[test]
2131+
fn verbose_show_failed_fix_errors() {
2132+
let mut cmd = RuffCheck::default()
2133+
.args(["--select", "UP006", "--preview", "-v"])
2134+
.build();
2135+
2136+
insta::with_settings!(
2137+
{
2138+
// the logs have timestamps we need to remove
2139+
filters => vec![(
2140+
r"\[[\d:-]+]",
2141+
""
2142+
)]
2143+
},{
2144+
assert_cmd_snapshot!(cmd
2145+
.pass_stdin("import typing\nCallable = 'abc'\ndef g() -> typing.Callable: ..."),
2146+
@r###"
2147+
success: false
2148+
exit_code: 1
2149+
----- stdout -----
2150+
-:3:12: UP006 Use `collections.abc.Callable` instead of `typing.Callable` for type annotation
2151+
|
2152+
1 | import typing
2153+
2 | Callable = 'abc'
2154+
3 | def g() -> typing.Callable: ...
2155+
| ^^^^^^^^^^^^^^^ UP006
2156+
|
2157+
= help: Replace with `collections.abc.Callable`
2158+
2159+
Found 1 error.
2160+
2161+
----- stderr -----
2162+
[ruff::resolve][DEBUG] Isolated mode, not reading any pyproject.toml
2163+
[ruff_diagnostics::diagnostic][DEBUG] Failed to create fix for NonPEP585Annotation: Unable to insert `Callable` into scope due to name conflict
2164+
"###); }
2165+
);
2166+
}
2167+
2168+
#[test]
2169+
fn no_verbose_hide_failed_fix_errors() {
2170+
let mut cmd = RuffCheck::default()
2171+
.args(["--select", "UP006", "--preview"])
2172+
.build();
2173+
assert_cmd_snapshot!(cmd
2174+
.pass_stdin("import typing\nCallable = 'abc'\ndef g() -> typing.Callable: ..."),
2175+
@r###"
2176+
success: false
2177+
exit_code: 1
2178+
----- stdout -----
2179+
-:3:12: UP006 Use `collections.abc.Callable` instead of `typing.Callable` for type annotation
2180+
|
2181+
1 | import typing
2182+
2 | Callable = 'abc'
2183+
3 | def g() -> typing.Callable: ...
2184+
| ^^^^^^^^^^^^^^^ UP006
2185+
|
2186+
= help: Replace with `collections.abc.Callable`
2187+
2188+
Found 1 error.
2189+
2190+
----- stderr -----
2191+
"###);
2192+
}

‎crates/ruff_diagnostics/src/diagnostic.rs

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
use anyhow::Result;
2-
use log::error;
2+
use log::debug;
33
#[cfg(feature = "serde")]
44
use serde::{Deserialize, Serialize};
55

@@ -56,7 +56,7 @@ impl Diagnostic {
5656
pub fn try_set_fix(&mut self, func: impl FnOnce() -> Result<Fix>) {
5757
match func() {
5858
Ok(fix) => self.fix = Some(fix),
59-
Err(err) => error!("Failed to create fix for {}: {}", self.kind.name, err),
59+
Err(err) => debug!("Failed to create fix for {}: {}", self.kind.name, err),
6060
}
6161
}
6262

@@ -67,7 +67,7 @@ impl Diagnostic {
6767
match func() {
6868
Ok(None) => {}
6969
Ok(Some(fix)) => self.fix = Some(fix),
70-
Err(err) => error!("Failed to create fix for {}: {}", self.kind.name, err),
70+
Err(err) => debug!("Failed to create fix for {}: {}", self.kind.name, err),
7171
}
7272
}
7373

‎crates/ruff_linter/src/rules/flake8_simplify/rules/ast_with.rs

Lines changed: 23 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
1+
use anyhow::bail;
12
use ast::Expr;
2-
use log::error;
33

44
use ruff_diagnostics::{Diagnostic, Fix};
55
use ruff_diagnostics::{FixAvailability, Violation};
@@ -170,26 +170,30 @@ pub(crate) fn multiple_with_statements(
170170
.comment_ranges()
171171
.intersects(TextRange::new(with_stmt.start(), with_stmt.body[0].start()))
172172
{
173-
match fix_with::fix_multiple_with_statements(
174-
checker.locator(),
175-
checker.stylist(),
176-
with_stmt,
177-
) {
178-
Ok(edit) => {
179-
if edit.content().map_or(true, |content| {
180-
fits(
181-
content,
182-
with_stmt.into(),
183-
checker.locator(),
184-
checker.settings.pycodestyle.max_line_length,
185-
checker.settings.tab_size,
186-
)
187-
}) {
188-
diagnostic.set_fix(Fix::unsafe_edit(edit));
173+
diagnostic.try_set_optional_fix(|| {
174+
match fix_with::fix_multiple_with_statements(
175+
checker.locator(),
176+
checker.stylist(),
177+
with_stmt,
178+
) {
179+
Ok(edit) => {
180+
if edit.content().map_or(true, |content| {
181+
fits(
182+
content,
183+
with_stmt.into(),
184+
checker.locator(),
185+
checker.settings.pycodestyle.max_line_length,
186+
checker.settings.tab_size,
187+
)
188+
}) {
189+
Ok(Some(Fix::unsafe_edit(edit)))
190+
} else {
191+
Ok(None)
192+
}
189193
}
194+
Err(err) => bail!("Failed to collapse `with`: {err}"),
190195
}
191-
Err(err) => error!("Failed to fix nested with: {err}"),
192-
}
196+
});
193197
}
194198
checker.diagnostics.push(diagnostic);
195199
}

‎crates/ruff_linter/src/rules/flake8_simplify/rules/collapsible_if.rs

Lines changed: 18 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,6 @@ use std::borrow::Cow;
22

33
use anyhow::{bail, Result};
44
use libcst_native::ParenthesizedNode;
5-
use log::error;
65

76
use ruff_diagnostics::{Diagnostic, Edit, Fix, FixAvailability, Violation};
87
use ruff_macros::{derive_message_formats, ViolationMetadata};
@@ -118,22 +117,26 @@ pub(crate) fn nested_if_statements(
118117
nested_if.start(),
119118
nested_if.body()[0].start(),
120119
)) {
121-
match collapse_nested_if(checker.locator(), checker.stylist(), nested_if) {
122-
Ok(edit) => {
123-
if edit.content().map_or(true, |content| {
124-
fits(
125-
content,
126-
(&nested_if).into(),
127-
checker.locator(),
128-
checker.settings.pycodestyle.max_line_length,
129-
checker.settings.tab_size,
130-
)
131-
}) {
132-
diagnostic.set_fix(Fix::unsafe_edit(edit));
120+
diagnostic.try_set_optional_fix(|| {
121+
match collapse_nested_if(checker.locator(), checker.stylist(), nested_if) {
122+
Ok(edit) => {
123+
if edit.content().map_or(true, |content| {
124+
fits(
125+
content,
126+
(&nested_if).into(),
127+
checker.locator(),
128+
checker.settings.pycodestyle.max_line_length,
129+
checker.settings.tab_size,
130+
)
131+
}) {
132+
Ok(Some(Fix::unsafe_edit(edit)))
133+
} else {
134+
Ok(None)
135+
}
133136
}
137+
Err(err) => bail!("Failed to collapse `if`: {err}"),
134138
}
135-
Err(err) => error!("Failed to fix nested if: {err}"),
136-
}
139+
});
137140
}
138141
checker.diagnostics.push(diagnostic);
139142
}

‎crates/ruff_linter/src/rules/pyflakes/rules/invalid_literal_comparisons.rs

Lines changed: 22 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
use log::error;
1+
use anyhow::{bail, Error};
22

33
use ruff_diagnostics::{AlwaysFixableViolation, Diagnostic, Edit, Fix};
44
use ruff_macros::{derive_message_formats, ViolationMetadata};
@@ -94,24 +94,29 @@ pub(crate) fn invalid_literal_comparison(
9494
if lazy_located.is_none() {
9595
lazy_located = Some(locate_cmp_ops(expr, checker.tokens()));
9696
}
97-
if let Some(located_op) = lazy_located.as_ref().and_then(|located| located.get(index)) {
98-
assert_eq!(located_op.op, *op);
99-
if let Some(content) = match located_op.op {
100-
CmpOp::Is => Some("==".to_string()),
101-
CmpOp::IsNot => Some("!=".to_string()),
102-
node => {
103-
error!("Failed to fix invalid comparison: {node:?}");
104-
None
97+
diagnostic.try_set_optional_fix(|| {
98+
if let Some(located_op) =
99+
lazy_located.as_ref().and_then(|located| located.get(index))
100+
{
101+
assert_eq!(located_op.op, *op);
102+
if let Ok(content) = match located_op.op {
103+
CmpOp::Is => Ok::<String, Error>("==".to_string()),
104+
CmpOp::IsNot => Ok("!=".to_string()),
105+
node => {
106+
bail!("Failed to fix invalid comparison: {node:?}")
107+
}
108+
} {
109+
Ok(Some(Fix::safe_edit(Edit::range_replacement(
110+
content,
111+
located_op.range,
112+
))))
113+
} else {
114+
Ok(None)
105115
}
106-
} {
107-
diagnostic.set_fix(Fix::safe_edit(Edit::range_replacement(
108-
content,
109-
located_op.range,
110-
)));
116+
} else {
117+
bail!("Failed to fix invalid comparison due to missing op")
111118
}
112-
} else {
113-
error!("Failed to fix invalid comparison due to missing op");
114-
}
119+
});
115120
checker.diagnostics.push(diagnostic);
116121
}
117122
left = right;

‎crates/ruff_linter/src/rules/pyupgrade/rules/deprecated_mock_import.rs

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,7 @@ use libcst_native::{
33
AsName, AssignTargetExpression, Attribute, Dot, Expression, Import, ImportAlias, ImportFrom,
44
ImportNames, Name, NameOrAttribute, ParenthesizableWhitespace,
55
};
6-
use log::error;
6+
use log::debug;
77

88
use ruff_diagnostics::{AlwaysFixableViolation, Diagnostic, Edit, Fix};
99
use ruff_macros::{derive_message_formats, ViolationMetadata};
@@ -286,7 +286,7 @@ pub(crate) fn deprecated_mock_import(checker: &mut Checker, stmt: &Stmt) {
286286
match format_import(stmt, indent, checker.locator(), checker.stylist()) {
287287
Ok(content) => Some(content),
288288
Err(e) => {
289-
error!("Failed to rewrite `mock` import: {e}");
289+
debug!("Failed to rewrite `mock` import: {e}");
290290
None
291291
}
292292
}

0 commit comments

Comments
 (0)