From 732b0405d7f00e09080b6650e157805924c447d3 Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Wed, 14 Jun 2023 11:17:09 -0400 Subject: [PATCH] Remove `FixMode::None` (#5087) ## Summary We now _always_ generate fixes, so `FixMode::None` and `FixMode::Generate` are redundant. We can also remove the TODO around `--fix-dry-run`, since that's our default behavior. Closes #5081. --- crates/ruff/src/settings/flags.rs | 13 +----- crates/ruff_cli/src/commands/run.rs | 8 ++-- crates/ruff_cli/src/diagnostics.rs | 66 +++++++++++++++-------------- crates/ruff_cli/src/lib.rs | 12 ++---- crates/ruff_cli/src/printer.rs | 6 +-- 5 files changed, 46 insertions(+), 59 deletions(-) diff --git a/crates/ruff/src/settings/flags.rs b/crates/ruff/src/settings/flags.rs index f7e761ade1..a1ce403194 100644 --- a/crates/ruff/src/settings/flags.rs +++ b/crates/ruff/src/settings/flags.rs @@ -1,19 +1,8 @@ -#[derive(Debug, Copy, Clone, Hash)] +#[derive(Debug, Copy, Clone, Hash, is_macro::Is)] pub enum FixMode { Generate, Apply, Diff, - None, -} - -impl From for FixMode { - fn from(value: bool) -> Self { - if value { - Self::Apply - } else { - Self::None - } - } } #[derive(Debug, Copy, Clone, Hash, result_like::BoolLike)] diff --git a/crates/ruff_cli/src/commands/run.rs b/crates/ruff_cli/src/commands/run.rs index 78ef048870..c299be0c09 100644 --- a/crates/ruff_cli/src/commands/run.rs +++ b/crates/ruff_cli/src/commands/run.rs @@ -247,21 +247,21 @@ mod test { &overrides, Cache::Disabled, Noqa::Enabled, - FixMode::None, + FixMode::Generate, )?; let printer = Printer::new( SerializationFormat::Text, LogLevel::Default, - FixMode::None, + FixMode::Generate, Flags::SHOW_VIOLATIONS, ); let mut writer: Vec = Vec::new(); - // Mute the terminal color codes + // Mute the terminal color codes. colored::control::set_override(false); printer.write_once(&diagnostics, &mut writer)?; // TODO(konstin): Set jupyter notebooks as none-fixable for now - // TODO(konstin) 2: Make jupyter notebooks fixable + // TODO(konstin): Make jupyter notebooks fixable let expected = format!( "{valid_ipynb}:cell 1:2:5: F841 [*] Local variable `x` is assigned to but never used {valid_ipynb}:cell 3:1:24: B006 Do not use mutable data structures for argument defaults diff --git a/crates/ruff_cli/src/diagnostics.rs b/crates/ruff_cli/src/diagnostics.rs index 7a5ffa49d2..f8efe174c2 100644 --- a/crates/ruff_cli/src/diagnostics.rs +++ b/crates/ruff_cli/src/diagnostics.rs @@ -110,10 +110,7 @@ pub(crate) fn lint_path( // to cache `fixer::Mode::Apply`, since a file either has no fixes, or we'll // write the fixes to disk, thus invalidating the cache. But it's a bit hard // to reason about. We need to come up with a better solution here.) - let metadata = if cache.into() - && noqa.into() - && matches!(autofix, flags::FixMode::None | flags::FixMode::Generate) - { + let metadata = if cache.into() && noqa.into() && autofix.is_generate() { let metadata = path.metadata()?; if let Some((messages, imports)) = cache::get(path, package, &metadata, settings) { debug!("Cache hit for: {}", path.display()); @@ -170,23 +167,25 @@ pub(crate) fn lint_path( &mut source_kind, ) { if !fixed.is_empty() { - if matches!(autofix, flags::FixMode::Apply) { - match &source_kind { + match autofix { + flags::FixMode::Apply => match &source_kind { SourceKind::Python(_) => { write(path, transformed.as_bytes())?; } SourceKind::Jupyter(notebook) => { notebook.write(path)?; } + }, + flags::FixMode::Diff => { + let mut stdout = io::stdout().lock(); + TextDiff::from_lines(contents.as_str(), &transformed) + .unified_diff() + .header(&fs::relativize_path(path), &fs::relativize_path(path)) + .to_writer(&mut stdout)?; + stdout.write_all(b"\n")?; + stdout.flush()?; } - } else if matches!(autofix, flags::FixMode::Diff) { - let mut stdout = io::stdout().lock(); - TextDiff::from_lines(contents.as_str(), &transformed) - .unified_diff() - .header(&fs::relativize_path(path), &fs::relativize_path(path)) - .to_writer(&mut stdout)?; - stdout.write_all(b"\n")?; - stdout.flush()?; + flags::FixMode::Generate => {} } } (result, fixed) @@ -269,23 +268,28 @@ pub(crate) fn lint_stdin( settings, &mut source_kind, ) { - if matches!(autofix, flags::FixMode::Apply) { - // Write the contents to stdout, regardless of whether any errors were fixed. - io::stdout().write_all(transformed.as_bytes())?; - } else if matches!(autofix, flags::FixMode::Diff) { - // But only write a diff if it's non-empty. - if !fixed.is_empty() { - let text_diff = TextDiff::from_lines(contents, &transformed); - let mut unified_diff = text_diff.unified_diff(); - if let Some(path) = path { - unified_diff.header(&fs::relativize_path(path), &fs::relativize_path(path)); - } - - let mut stdout = io::stdout().lock(); - unified_diff.to_writer(&mut stdout)?; - stdout.write_all(b"\n")?; - stdout.flush()?; + match autofix { + flags::FixMode::Apply => { + // Write the contents to stdout, regardless of whether any errors were fixed. + io::stdout().write_all(transformed.as_bytes())?; } + flags::FixMode::Diff => { + // But only write a diff if it's non-empty. + if !fixed.is_empty() { + let text_diff = TextDiff::from_lines(contents, &transformed); + let mut unified_diff = text_diff.unified_diff(); + if let Some(path) = path { + unified_diff + .header(&fs::relativize_path(path), &fs::relativize_path(path)); + } + + let mut stdout = io::stdout().lock(); + unified_diff.to_writer(&mut stdout)?; + stdout.write_all(b"\n")?; + stdout.flush()?; + } + } + flags::FixMode::Generate => {} } (result, fixed) @@ -301,7 +305,7 @@ pub(crate) fn lint_stdin( let fixed = FxHashMap::default(); // Write the contents to stdout anyway. - if matches!(autofix, flags::FixMode::Apply) { + if autofix.is_apply() { io::stdout().write_all(contents.as_bytes())?; } diff --git a/crates/ruff_cli/src/lib.rs b/crates/ruff_cli/src/lib.rs index 7f7a70f2eb..a9b53365fb 100644 --- a/crates/ruff_cli/src/lib.rs +++ b/crates/ruff_cli/src/lib.rs @@ -192,23 +192,17 @@ fn check(args: CheckArgs, log_level: LogLevel) -> Result { } = pyproject_config.settings.cli; // Autofix rules are as follows: + // - By default, generate all fixes, but don't apply them to the filesystem. // - If `--fix` or `--fix-only` is set, always apply fixes to the filesystem (or // print them to stdout, if we're reading from stdin). - // - Otherwise, if `--format json` is set, generate the fixes (so we print them - // out as part of the JSON payload), but don't write them to disk. // - If `--diff` or `--fix-only` are set, don't print any violations (only // fixes). - // TODO(charlie): Consider adding ESLint's `--fix-dry-run`, which would generate - // but not apply fixes. That would allow us to avoid special-casing JSON - // here. let autofix = if cli.diff { flags::FixMode::Diff } else if fix || fix_only { flags::FixMode::Apply - } else if matches!(format, SerializationFormat::Json) { - flags::FixMode::Generate } else { - flags::FixMode::None + flags::FixMode::Generate }; let cache = !cli.no_cache; let noqa = !cli.ignore_noqa; @@ -238,7 +232,7 @@ fn check(args: CheckArgs, log_level: LogLevel) -> Result { } if cli.add_noqa { - if !matches!(autofix, flags::FixMode::None) { + if !autofix.is_generate() { warn_user_once!("--fix is incompatible with --add-noqa."); } let modifications = diff --git a/crates/ruff_cli/src/printer.rs b/crates/ruff_cli/src/printer.rs index 707460675f..2062b9e823 100644 --- a/crates/ruff_cli/src/printer.rs +++ b/crates/ruff_cli/src/printer.rs @@ -141,9 +141,9 @@ impl Printer { .sum::(); if fixed > 0 { let s = if fixed == 1 { "" } else { "s" }; - if matches!(self.autofix_level, flags::FixMode::Apply) { + if self.autofix_level.is_apply() { writeln!(stdout, "Fixed {fixed} error{s}.")?; - } else if matches!(self.autofix_level, flags::FixMode::Diff) { + } else { writeln!(stdout, "Would fix {fixed} error{s}.")?; } } @@ -391,7 +391,7 @@ const fn show_fix_status(autofix_level: flags::FixMode) -> bool { // this pass! (We're occasionally unable to determine whether a specific // violation is fixable without trying to fix it, so if autofix is not // enabled, we may inadvertently indicate that a rule is fixable.) - !matches!(autofix_level, flags::FixMode::Apply) + !autofix_level.is_apply() } fn print_fix_summary(stdout: &mut T, fixed: &FxHashMap) -> Result<()> {