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.
This commit is contained in:
Charlie Marsh 2023-06-14 11:17:09 -04:00 committed by GitHub
parent e7316c1cc6
commit 732b0405d7
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
5 changed files with 46 additions and 59 deletions

View file

@ -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<bool> for FixMode {
fn from(value: bool) -> Self {
if value {
Self::Apply
} else {
Self::None
}
}
}
#[derive(Debug, Copy, Clone, Hash, result_like::BoolLike)]

View file

@ -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<u8> = 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

View file

@ -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())?;
}

View file

@ -192,23 +192,17 @@ fn check(args: CheckArgs, log_level: LogLevel) -> Result<ExitStatus> {
} = 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<ExitStatus> {
}
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 =

View file

@ -141,9 +141,9 @@ impl Printer {
.sum::<usize>();
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<T: Write>(stdout: &mut T, fixed: &FxHashMap<String, FixTable>) -> Result<()> {