From 2d6557a51bfeb43ed1e922383ad066ce6626fec8 Mon Sep 17 00:00:00 2001 From: Zanie Blue Date: Sun, 8 Oct 2023 11:14:37 -0500 Subject: [PATCH] Only show warnings for empty preview selectors when enabling rules (#7842) Closes https://github.com/astral-sh/ruff/issues/7491 Users found it confusing that warnings were displayed when ignoring a preview rule (which has no effect without `--preview`). While we could retain the warning with different messaging, I've opted to remove it for now. With this pull request, we will only warn on `--select` and `--extend-select` but not `--fixable`, `--unfixable`, `--ignore`, or `--extend-fixable`. --- Cargo.lock | 1 + crates/ruff_cli/tests/integration_test.rs | 36 +++++++++++++ crates/ruff_workspace/Cargo.toml | 1 + crates/ruff_workspace/src/configuration.rs | 60 ++++++++++++++++++---- 4 files changed, 87 insertions(+), 11 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 3f2f82896658d2..2373aabb12f033 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2494,6 +2494,7 @@ dependencies = [ "glob", "globset", "ignore", + "is-macro", "itertools 0.11.0", "log", "once_cell", diff --git a/crates/ruff_cli/tests/integration_test.rs b/crates/ruff_cli/tests/integration_test.rs index 13aeae1d4f97b0..07275021c8f22c 100644 --- a/crates/ruff_cli/tests/integration_test.rs +++ b/crates/ruff_cli/tests/integration_test.rs @@ -737,6 +737,42 @@ fn preview_disabled_prefix_empty() { "###); } +#[test] +fn preview_disabled_does_not_warn_for_empty_ignore_selections() { + // Does not warn that the selection is empty since the user is not trying to enable the rule + let args = ["--ignore", "CPY"]; + assert_cmd_snapshot!(Command::new(get_cargo_bin(BIN_NAME)) + .args(STDIN_BASE_OPTIONS) + .args(args) + .pass_stdin("I=42\n"), @r###" + success: false + exit_code: 1 + ----- stdout ----- + -:1:1: E741 Ambiguous variable name: `I` + Found 1 error. + + ----- stderr ----- + "###); +} + +#[test] +fn preview_disabled_does_not_warn_for_empty_fixable_selections() { + // Does not warn that the selection is empty since the user is not trying to enable the rule + let args = ["--fixable", "CPY"]; + assert_cmd_snapshot!(Command::new(get_cargo_bin(BIN_NAME)) + .args(STDIN_BASE_OPTIONS) + .args(args) + .pass_stdin("I=42\n"), @r###" + success: false + exit_code: 1 + ----- stdout ----- + -:1:1: E741 Ambiguous variable name: `I` + Found 1 error. + + ----- stderr ----- + "###); +} + #[test] fn preview_group_selector() { // `--select PREVIEW` should error (selector was removed) diff --git a/crates/ruff_workspace/Cargo.toml b/crates/ruff_workspace/Cargo.toml index 227dca554372b9..154fb55d1b0869 100644 --- a/crates/ruff_workspace/Cargo.toml +++ b/crates/ruff_workspace/Cargo.toml @@ -25,6 +25,7 @@ anyhow = { workspace = true } colored = { workspace = true } dirs = { version = "5.0.0" } ignore = { workspace = true } +is-macro = { workspace = true } itertools = { workspace = true } log = { workspace = true } glob = { workspace = true } diff --git a/crates/ruff_workspace/src/configuration.rs b/crates/ruff_workspace/src/configuration.rs index 9733bbf197bdfd..d480ec3e73c570 100644 --- a/crates/ruff_workspace/src/configuration.rs +++ b/crates/ruff_workspace/src/configuration.rs @@ -57,6 +57,51 @@ pub struct RuleSelection { pub extend_fixable: Vec, } +#[derive(Debug, Eq, PartialEq, is_macro::Is)] +pub enum RuleSelectorKind { + /// Enables the selected rules + Enable, + /// Disables the selected rules + Disable, + /// Modifies the behavior of selected rules + Modify, +} + +impl RuleSelection { + pub fn selectors_by_kind(&self) -> impl Iterator { + self.select + .iter() + .flatten() + .map(|selector| (RuleSelectorKind::Enable, selector)) + .chain( + self.fixable + .iter() + .flatten() + .map(|selector| (RuleSelectorKind::Modify, selector)), + ) + .chain( + self.ignore + .iter() + .map(|selector| (RuleSelectorKind::Disable, selector)), + ) + .chain( + self.extend_select + .iter() + .map(|selector| (RuleSelectorKind::Enable, selector)), + ) + .chain( + self.unfixable + .iter() + .map(|selector| (RuleSelectorKind::Modify, selector)), + ) + .chain( + self.extend_fixable + .iter() + .map(|selector| (RuleSelectorKind::Modify, selector)), + ) + } +} + #[derive(Debug, Default)] pub struct Configuration { // Global options @@ -704,16 +749,7 @@ impl LintConfiguration { } // Check for selections that require a warning - for selector in selection - .select - .iter() - .chain(selection.fixable.iter()) - .flatten() - .chain(selection.ignore.iter()) - .chain(selection.extend_select.iter()) - .chain(selection.unfixable.iter()) - .chain(selection.extend_fixable.iter()) - { + for (kind, selector) in selection.selectors_by_kind() { #[allow(deprecated)] if matches!(selector, RuleSelector::Nursery) { let suggestion = if preview.mode.is_disabled() { @@ -725,7 +761,9 @@ impl LintConfiguration { warn_user_once!("The `NURSERY` selector has been deprecated.{suggestion}"); }; - if preview.mode.is_disabled() { + // Only warn for the following selectors if used to enable rules + // e.g. use with `--ignore` or `--fixable` is okay + if preview.mode.is_disabled() && kind.is_enable() { if let RuleSelector::Rule { prefix, .. } = selector { if prefix.rules().any(|rule| rule.is_nursery()) { deprecated_nursery_selectors.insert(selector);