From bb29c420a0a16a96483dfb153b8862ab49a63ae1 Mon Sep 17 00:00:00 2001 From: dylwil3 Date: Fri, 3 Jan 2025 00:42:10 -0600 Subject: [PATCH 1/5] test for desired behavior on stdout stderr --- crates/ruff/tests/integration_test.rs | 57 +++++++++++++++++++++++++++ 1 file changed, 57 insertions(+) diff --git a/crates/ruff/tests/integration_test.rs b/crates/ruff/tests/integration_test.rs index bae45dd22f78a4..5dcb9bfad4e1df 100644 --- a/crates/ruff/tests/integration_test.rs +++ b/crates/ruff/tests/integration_test.rs @@ -2126,3 +2126,60 @@ unfixable = ["RUF"] Ok(()) } + +#[test] +fn verbose_show_failed_fix_errors() -> Result<()> { + let mut cmd = RuffCheck::default() + .args(["--select", "UP006", "--preview", "-v"]) + .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 ----- + [2025-01-03][00:40:50][ruff::resolve][DEBUG] Isolated mode, not reading any pyproject.toml + [2025-01-03][00:40:50][ruff_diagnostics::diagnostic][DEBUG] Failed to create fix for NonPEP585Annotation: Unable to insert `Callable` into scope due to name conflict + "###); + + Ok(()) +} +#[test] +fn no_verbose_hide_failed_fix_errors() -> Result<()> { + 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 ----- + "###); + + Ok(()) +} From 8d9c8013c9ae983bc6aaab383c819cb2d61814ed Mon Sep 17 00:00:00 2001 From: dylwil3 Date: Fri, 3 Jan 2025 00:42:53 -0600 Subject: [PATCH 2/5] use debug log level for try_set_fix --- crates/ruff_diagnostics/src/diagnostic.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/crates/ruff_diagnostics/src/diagnostic.rs b/crates/ruff_diagnostics/src/diagnostic.rs index 84da5f3904607d..be54a8de0e2475 100644 --- a/crates/ruff_diagnostics/src/diagnostic.rs +++ b/crates/ruff_diagnostics/src/diagnostic.rs @@ -1,5 +1,5 @@ use anyhow::Result; -use log::error; +use log::debug; #[cfg(feature = "serde")] use serde::{Deserialize, Serialize}; @@ -56,7 +56,7 @@ impl Diagnostic { pub fn try_set_fix(&mut self, func: impl FnOnce() -> Result) { 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), } } @@ -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), } } From c9deccee8f7be4e02bd032d18abcf746bc12d83e Mon Sep 17 00:00:00 2001 From: dylwil3 Date: Fri, 3 Jan 2025 00:43:28 -0600 Subject: [PATCH 3/5] replace ad hoc error macro uses with try_set_fix or debug --- .../rules/flake8_simplify/rules/ast_with.rs | 42 ++++++++++--------- .../flake8_simplify/rules/collapsible_if.rs | 33 ++++++++------- .../rules/invalid_literal_comparisons.rs | 39 +++++++++-------- .../pyupgrade/rules/deprecated_mock_import.rs | 4 +- 4 files changed, 65 insertions(+), 53 deletions(-) diff --git a/crates/ruff_linter/src/rules/flake8_simplify/rules/ast_with.rs b/crates/ruff_linter/src/rules/flake8_simplify/rules/ast_with.rs index 7bfe65c9295b6f..77a2c8bdd5046c 100644 --- a/crates/ruff_linter/src/rules/flake8_simplify/rules/ast_with.rs +++ b/crates/ruff_linter/src/rules/flake8_simplify/rules/ast_with.rs @@ -1,5 +1,5 @@ +use anyhow::bail; use ast::Expr; -use log::error; use ruff_diagnostics::{Diagnostic, Fix}; use ruff_diagnostics::{FixAvailability, Violation}; @@ -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); } diff --git a/crates/ruff_linter/src/rules/flake8_simplify/rules/collapsible_if.rs b/crates/ruff_linter/src/rules/flake8_simplify/rules/collapsible_if.rs index c32ced436b5249..db8594df698720 100644 --- a/crates/ruff_linter/src/rules/flake8_simplify/rules/collapsible_if.rs +++ b/crates/ruff_linter/src/rules/flake8_simplify/rules/collapsible_if.rs @@ -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}; @@ -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); } diff --git a/crates/ruff_linter/src/rules/pyflakes/rules/invalid_literal_comparisons.rs b/crates/ruff_linter/src/rules/pyflakes/rules/invalid_literal_comparisons.rs index 955def2f9a8925..71c10a8adf76df 100644 --- a/crates/ruff_linter/src/rules/pyflakes/rules/invalid_literal_comparisons.rs +++ b/crates/ruff_linter/src/rules/pyflakes/rules/invalid_literal_comparisons.rs @@ -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}; @@ -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::("==".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; diff --git a/crates/ruff_linter/src/rules/pyupgrade/rules/deprecated_mock_import.rs b/crates/ruff_linter/src/rules/pyupgrade/rules/deprecated_mock_import.rs index 4a0c6998e3eacd..92268c8ee77027 100644 --- a/crates/ruff_linter/src/rules/pyupgrade/rules/deprecated_mock_import.rs +++ b/crates/ruff_linter/src/rules/pyupgrade/rules/deprecated_mock_import.rs @@ -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}; @@ -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 } } From 4a8e46f239b4f6128a60854016033a007fd24709 Mon Sep 17 00:00:00 2001 From: dylwil3 Date: Fri, 3 Jan 2025 00:47:47 -0600 Subject: [PATCH 4/5] clippy --- crates/ruff/tests/integration_test.rs | 9 +++------ .../src/rules/flake8_simplify/rules/ast_with.rs | 2 +- .../src/rules/flake8_simplify/rules/collapsible_if.rs | 2 +- 3 files changed, 5 insertions(+), 8 deletions(-) diff --git a/crates/ruff/tests/integration_test.rs b/crates/ruff/tests/integration_test.rs index 5dcb9bfad4e1df..a1f1dc144e4ee7 100644 --- a/crates/ruff/tests/integration_test.rs +++ b/crates/ruff/tests/integration_test.rs @@ -2128,7 +2128,7 @@ unfixable = ["RUF"] } #[test] -fn verbose_show_failed_fix_errors() -> Result<()> { +fn verbose_show_failed_fix_errors() { let mut cmd = RuffCheck::default() .args(["--select", "UP006", "--preview", "-v"]) .build(); @@ -2153,11 +2153,10 @@ fn verbose_show_failed_fix_errors() -> Result<()> { [2025-01-03][00:40:50][ruff::resolve][DEBUG] Isolated mode, not reading any pyproject.toml [2025-01-03][00:40:50][ruff_diagnostics::diagnostic][DEBUG] Failed to create fix for NonPEP585Annotation: Unable to insert `Callable` into scope due to name conflict "###); - - Ok(()) } + #[test] -fn no_verbose_hide_failed_fix_errors() -> Result<()> { +fn no_verbose_hide_failed_fix_errors() { let mut cmd = RuffCheck::default() .args(["--select", "UP006", "--preview"]) .build(); @@ -2180,6 +2179,4 @@ fn no_verbose_hide_failed_fix_errors() -> Result<()> { ----- stderr ----- "###); - - Ok(()) } diff --git a/crates/ruff_linter/src/rules/flake8_simplify/rules/ast_with.rs b/crates/ruff_linter/src/rules/flake8_simplify/rules/ast_with.rs index 77a2c8bdd5046c..906aff0c25a532 100644 --- a/crates/ruff_linter/src/rules/flake8_simplify/rules/ast_with.rs +++ b/crates/ruff_linter/src/rules/flake8_simplify/rules/ast_with.rs @@ -193,7 +193,7 @@ pub(crate) fn multiple_with_statements( } Err(err) => bail!("Failed to collapse `with`: {err}"), } - }) + }); } checker.diagnostics.push(diagnostic); } diff --git a/crates/ruff_linter/src/rules/flake8_simplify/rules/collapsible_if.rs b/crates/ruff_linter/src/rules/flake8_simplify/rules/collapsible_if.rs index db8594df698720..6ede86a536585e 100644 --- a/crates/ruff_linter/src/rules/flake8_simplify/rules/collapsible_if.rs +++ b/crates/ruff_linter/src/rules/flake8_simplify/rules/collapsible_if.rs @@ -136,7 +136,7 @@ pub(crate) fn nested_if_statements( } Err(err) => bail!("Failed to collapse `if`: {err}"), } - }) + }); } checker.diagnostics.push(diagnostic); } From cf92b9b4f0014c324496cd49b58034d67b8d45fd Mon Sep 17 00:00:00 2001 From: dylwil3 Date: Fri, 3 Jan 2025 01:22:29 -0600 Subject: [PATCH 5/5] filter out timestamp from cmd snapshot --- crates/ruff/tests/integration_test.rs | 20 +++++++++++++++----- 1 file changed, 15 insertions(+), 5 deletions(-) diff --git a/crates/ruff/tests/integration_test.rs b/crates/ruff/tests/integration_test.rs index a1f1dc144e4ee7..6001cc90452bda 100644 --- a/crates/ruff/tests/integration_test.rs +++ b/crates/ruff/tests/integration_test.rs @@ -2132,9 +2132,18 @@ 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###" + .pass_stdin("import typing\nCallable = 'abc'\ndef g() -> typing.Callable: ..."), + @r###" success: false exit_code: 1 ----- stdout ----- @@ -2150,9 +2159,10 @@ fn verbose_show_failed_fix_errors() { Found 1 error. ----- stderr ----- - [2025-01-03][00:40:50][ruff::resolve][DEBUG] Isolated mode, not reading any pyproject.toml - [2025-01-03][00:40:50][ruff_diagnostics::diagnostic][DEBUG] Failed to create fix for NonPEP585Annotation: Unable to insert `Callable` into scope due to name conflict - "###); + [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]