Skip to content

Commit

Permalink
Only show warnings for empty preview selectors when enabling rules (#…
Browse files Browse the repository at this point in the history
…7842)

Closes #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`.
  • Loading branch information
zanieb authored Oct 8, 2023
1 parent 2ba5677 commit 2d6557a
Show file tree
Hide file tree
Showing 4 changed files with 87 additions and 11 deletions.
1 change: 1 addition & 0 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

36 changes: 36 additions & 0 deletions crates/ruff_cli/tests/integration_test.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
1 change: 1 addition & 0 deletions crates/ruff_workspace/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -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 }
Expand Down
60 changes: 49 additions & 11 deletions crates/ruff_workspace/src/configuration.rs
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,51 @@ pub struct RuleSelection {
pub extend_fixable: Vec<RuleSelector>,
}

#[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<Item = (RuleSelectorKind, &RuleSelector)> {
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
Expand Down Expand Up @@ -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() {
Expand All @@ -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);
Expand Down

0 comments on commit 2d6557a

Please sign in to comment.