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

Show errors for attempted fixes only when passed --verbose #15237

Merged
merged 5 commits into from
Jan 3, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
64 changes: 64 additions & 0 deletions crates/ruff/tests/integration_test.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2126,3 +2126,67 @@ unfixable = ["RUF"]

Ok(())
}

#[test]
fn verbose_show_failed_fix_errors() {
let mut cmd = RuffCheck::default()
.args(["--select", "UP006", "--preview", "-v"])
.build();

insta::with_settings!(
{
// the logs have timestamps we need to remove
filters => vec![(
r"\[[\d:-]+]",
""
)]
},{
assert_cmd_snapshot!(cmd
.pass_stdin("import typing\nCallable = 'abc'\ndef g() -> typing.Callable: ..."),
@r###"
success: false
exit_code: 1
----- stdout -----
-:3:12: UP006 Use `collections.abc.Callable` instead of `typing.Callable` for type annotation
|
1 | import typing
2 | Callable = 'abc'
3 | def g() -> typing.Callable: ...
| ^^^^^^^^^^^^^^^ UP006
|
= help: Replace with `collections.abc.Callable`

Found 1 error.

----- stderr -----
[ruff::resolve][DEBUG] Isolated mode, not reading any pyproject.toml
[ruff_diagnostics::diagnostic][DEBUG] Failed to create fix for NonPEP585Annotation: Unable to insert `Callable` into scope due to name conflict
"###); }
);
}

#[test]
fn no_verbose_hide_failed_fix_errors() {
let mut cmd = RuffCheck::default()
.args(["--select", "UP006", "--preview"])
.build();
assert_cmd_snapshot!(cmd
.pass_stdin("import typing\nCallable = 'abc'\ndef g() -> typing.Callable: ..."),
@r###"
success: false
exit_code: 1
----- stdout -----
-:3:12: UP006 Use `collections.abc.Callable` instead of `typing.Callable` for type annotation
|
1 | import typing
2 | Callable = 'abc'
3 | def g() -> typing.Callable: ...
| ^^^^^^^^^^^^^^^ UP006
|
= help: Replace with `collections.abc.Callable`

Found 1 error.

----- stderr -----
"###);
}
6 changes: 3 additions & 3 deletions crates/ruff_diagnostics/src/diagnostic.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
use anyhow::Result;
use log::error;
use log::debug;
#[cfg(feature = "serde")]
use serde::{Deserialize, Serialize};

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

Expand All @@ -67,7 +67,7 @@ impl Diagnostic {
match func() {
Ok(None) => {}
Ok(Some(fix)) => self.fix = Some(fix),
Err(err) => error!("Failed to create fix for {}: {}", self.kind.name, err),
Err(err) => debug!("Failed to create fix for {}: {}", self.kind.name, err),
}
}

Expand Down
42 changes: 23 additions & 19 deletions crates/ruff_linter/src/rules/flake8_simplify/rules/ast_with.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
use anyhow::bail;
use ast::Expr;
use log::error;

use ruff_diagnostics::{Diagnostic, Fix};
use ruff_diagnostics::{FixAvailability, Violation};
Expand Down Expand Up @@ -170,26 +170,30 @@ pub(crate) fn multiple_with_statements(
.comment_ranges()
.intersects(TextRange::new(with_stmt.start(), with_stmt.body[0].start()))
{
match fix_with::fix_multiple_with_statements(
checker.locator(),
checker.stylist(),
with_stmt,
) {
Ok(edit) => {
if edit.content().map_or(true, |content| {
fits(
content,
with_stmt.into(),
checker.locator(),
checker.settings.pycodestyle.max_line_length,
checker.settings.tab_size,
)
}) {
diagnostic.set_fix(Fix::unsafe_edit(edit));
diagnostic.try_set_optional_fix(|| {
match fix_with::fix_multiple_with_statements(
checker.locator(),
checker.stylist(),
with_stmt,
) {
Ok(edit) => {
if edit.content().map_or(true, |content| {
fits(
content,
with_stmt.into(),
checker.locator(),
checker.settings.pycodestyle.max_line_length,
checker.settings.tab_size,
)
}) {
Ok(Some(Fix::unsafe_edit(edit)))
} else {
Ok(None)
}
}
Err(err) => bail!("Failed to collapse `with`: {err}"),
}
Err(err) => error!("Failed to fix nested with: {err}"),
}
});
}
checker.diagnostics.push(diagnostic);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@ use std::borrow::Cow;

use anyhow::{bail, Result};
use libcst_native::ParenthesizedNode;
use log::error;

use ruff_diagnostics::{Diagnostic, Edit, Fix, FixAvailability, Violation};
use ruff_macros::{derive_message_formats, ViolationMetadata};
Expand Down Expand Up @@ -118,22 +117,26 @@ pub(crate) fn nested_if_statements(
nested_if.start(),
nested_if.body()[0].start(),
)) {
match collapse_nested_if(checker.locator(), checker.stylist(), nested_if) {
Ok(edit) => {
if edit.content().map_or(true, |content| {
fits(
content,
(&nested_if).into(),
checker.locator(),
checker.settings.pycodestyle.max_line_length,
checker.settings.tab_size,
)
}) {
diagnostic.set_fix(Fix::unsafe_edit(edit));
diagnostic.try_set_optional_fix(|| {
match collapse_nested_if(checker.locator(), checker.stylist(), nested_if) {
Ok(edit) => {
if edit.content().map_or(true, |content| {
fits(
content,
(&nested_if).into(),
checker.locator(),
checker.settings.pycodestyle.max_line_length,
checker.settings.tab_size,
)
}) {
Ok(Some(Fix::unsafe_edit(edit)))
} else {
Ok(None)
}
}
Err(err) => bail!("Failed to collapse `if`: {err}"),
}
Err(err) => error!("Failed to fix nested if: {err}"),
}
});
}
checker.diagnostics.push(diagnostic);
}
Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
use log::error;
use anyhow::{bail, Error};

use ruff_diagnostics::{AlwaysFixableViolation, Diagnostic, Edit, Fix};
use ruff_macros::{derive_message_formats, ViolationMetadata};
Expand Down Expand Up @@ -94,24 +94,29 @@ pub(crate) fn invalid_literal_comparison(
if lazy_located.is_none() {
lazy_located = Some(locate_cmp_ops(expr, checker.tokens()));
}
if let Some(located_op) = lazy_located.as_ref().and_then(|located| located.get(index)) {
assert_eq!(located_op.op, *op);
if let Some(content) = match located_op.op {
CmpOp::Is => Some("==".to_string()),
CmpOp::IsNot => Some("!=".to_string()),
node => {
error!("Failed to fix invalid comparison: {node:?}");
None
diagnostic.try_set_optional_fix(|| {
if let Some(located_op) =
lazy_located.as_ref().and_then(|located| located.get(index))
{
assert_eq!(located_op.op, *op);
if let Ok(content) = match located_op.op {
CmpOp::Is => Ok::<String, Error>("==".to_string()),
CmpOp::IsNot => Ok("!=".to_string()),
node => {
bail!("Failed to fix invalid comparison: {node:?}")
}
} {
Ok(Some(Fix::safe_edit(Edit::range_replacement(
content,
located_op.range,
))))
} else {
Ok(None)
}
} {
diagnostic.set_fix(Fix::safe_edit(Edit::range_replacement(
content,
located_op.range,
)));
} else {
bail!("Failed to fix invalid comparison due to missing op")
}
} else {
error!("Failed to fix invalid comparison due to missing op");
}
});
checker.diagnostics.push(diagnostic);
}
left = right;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ use libcst_native::{
AsName, AssignTargetExpression, Attribute, Dot, Expression, Import, ImportAlias, ImportFrom,
ImportNames, Name, NameOrAttribute, ParenthesizableWhitespace,
};
use log::error;
use log::debug;

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