Skip to content

Commit

Permalink
Have CLI repsect rule applicability when fixing
Browse files Browse the repository at this point in the history
  • Loading branch information
@evanrittenhouse
evanrittenhouse committed Sep 24, 2023
1 parent 1a22eae commit ec2edd0
Show file tree
Hide file tree
Showing 15 changed files with 561 additions and 99 deletions.
11 changes: 9 additions & 2 deletions crates/ruff_cli/src/args.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -72,15 +72,17 @@ pub enum Command {

// The `Parser` derive is for ruff_dev, for ruff_cli `Args` would be sufficient
#[derive(Clone, Debug, clap::Parser)]
#[allow(clippy::struct_excessive_bools)]
pub struct CheckCommand {
/// List of files or directories to check.
pub files: Vec<PathBuf>,
/// Attempt to automatically fix lint violations.
/// Use `--no-fix` to disable.
#[arg(long, overrides_with("no_fix"))]
fix: bool,
#[clap(long, overrides_with("fix"), hide = true)]
/// Attempt to automatically fix both automatic and suggested lint violations.
#[arg(long, overrides_with_all(["fix", "no_fix"]))]
fix_suggested: bool,
#[clap(long, overrides_with_all(["fix", "fix_suggested"]), hide = true)]
no_fix: bool,
/// Show violations with source code.
/// Use `--no-show-source` to disable.
Expand Down Expand Up @@ -497,6 +499,7 @@ impl CheckCommand {
cache_dir: self.cache_dir,
fix: resolve_bool_arg(self.fix, self.no_fix),
fix_only: resolve_bool_arg(self.fix_only, self.no_fix_only),
fix_suggested: Some(self.fix_suggested),
force_exclude: resolve_bool_arg(self.force_exclude, self.no_force_exclude),
output_format: self.output_format.or(self.format),
show_fixes: resolve_bool_arg(self.show_fixes, self.no_show_fixes),
Expand Down Expand Up @@ -599,6 +602,7 @@ pub struct CliOverrides {
pub cache_dir: Option<PathBuf>,
pub fix: Option<bool>,
pub fix_only: Option<bool>,
pub fix_suggested: Option<bool>,
pub force_exclude: Option<bool>,
pub output_format: Option<SerializationFormat>,
pub show_fixes: Option<bool>,
Expand All @@ -624,6 +628,9 @@ impl ConfigurationTransformer for CliOverrides {
if let Some(fix_only) = &self.fix_only {
config.fix_only = Some(*fix_only);
}
if self.fix_suggested.is_some() {
config.fix_suggested = self.fix_suggested;
}
config.rule_selections.push(RuleSelection {
select: self.select.clone(),
ignore: self
Expand Down
6 changes: 3 additions & 3 deletions crates/ruff_cli/src/cache.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -409,7 +409,7 @@ mod tests {
&settings.linter,
Some(&cache),
flags::Noqa::Enabled,
flags::FixMode::Generate,
flags::FixMode::Generate(flags::SuggestedFixes::Apply),
)
.unwrap();
if diagnostics
Expand Down Expand Up @@ -454,7 +454,7 @@ mod tests {
&settings.linter,
Some(&cache),
flags::Noqa::Enabled,
flags::FixMode::Generate,
flags::FixMode::Generate(flags::SuggestedFixes::Apply),
)
.unwrap();
}
Expand Down Expand Up @@ -711,7 +711,7 @@ mod tests {
&self.settings.linter,
Some(cache),
flags::Noqa::Enabled,
flags::FixMode::Generate,
flags::FixMode::Generate(flags::SuggestedFixes::Apply),
)
}
}
Expand Down
2 changes: 1 addition & 1 deletion crates/ruff_cli/src/commands/check.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -284,7 +284,7 @@ mod test {
&CliOverrides::default(),
flags::Cache::Disabled,
flags::Noqa::Disabled,
flags::FixMode::Generate,
flags::FixMode::Generate(flags::SuggestedFixes::Apply),
)
.unwrap();
let mut output = Vec::new();
Expand Down
125 changes: 88 additions & 37 deletions crates/ruff_cli/src/diagnostics.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -220,12 +220,12 @@ pub(crate) fn lint_path(
error: parse_error,
},
fixed,
) = if matches!(autofix, flags::FixMode::Apply | flags::FixMode::Diff) {
) = if matches!(autofix, flags::FixMode::Apply(fix_suggested) | flags::FixMode::Diff(fix_suggested)) {
if let Ok(FixerResult {
result,
transformed,
fixed,
}) = lint_fix(path, package, noqa, settings, &source_kind, source_type)
}) = lint_fix(path, package, noqa, settings, &source_kind, source_type, fix_suggested)
{
if !fixed.is_empty() {
match autofix {
Expand Down Expand Up @@ -265,40 +265,55 @@ pub(crate) fn lint_path(
else {
continue;
};
TextDiff::from_lines(
&src_code_cell.source.to_string(),
&dest_code_cell.source.to_string(),
)
.unified_diff()
// Jupyter notebook cells don't necessarily have a newline
// at the end. For example,
//
// ```python
// print("hello")
// ```
//
// For a cell containing the above code, there'll only be one line,
// and it won't have a newline at the end. If it did, there'd be
// two lines, and the second line would be empty:
//
// ```python
// print("hello")
//
// ```
.missing_newline_hint(false)
.header(
&format!("{}:cell {}", &fs::relativize_path(path), idx),
&format!("{}:cell {}", &fs::relativize_path(path), idx),
)
.to_writer(&mut stdout)?;
TextDiff::from_lines(
&src_code_cell.source.to_string(),
&dest_code_cell.source.to_string(),
)
.unified_diff()
// Jupyter notebook cells don't necessarily have a newline
// at the end. For example,
//
// ```python
// print("hello")
// ```
//
// For a cell containing the above code, there'll only be one line,
// and it won't have a newline at the end. If it did, there'd be
// two lines, and the second line would be empty:
//
// ```python
// print("hello")
//
// ```
.missing_newline_hint(false)
.header(
&format!("{}:cell {}", &fs::relativize_path(path), idx),
&format!("{}:cell {}", &fs::relativize_path(path), idx),
)
.to_writer(&mut stdout)?;
}
stdout.write_all(b"\n")?;
stdout.flush()?;
}
stdout.write_all(b"\n")?;
stdout.flush()?;
}
}
flags::FixMode::Generate(_) => {}
}
flags::FixMode::Generate => {}
}
(result, fixed)
} else {
// If we fail to autofix, lint the original source code.
let result = lint_only(
&contents,
path,
package,
&settings.lib,
noqa,
Some(&source_kind),
source_type,
);
let fixed = FxHashMap::default();
(result, fixed)
}
(result, fixed)
} else {
Expand Down Expand Up @@ -393,7 +408,7 @@ pub(crate) fn lint_stdin(
error: parse_error,
},
fixed,
) = if matches!(autofix, flags::FixMode::Apply | flags::FixMode::Diff) {
) = if matches!(autofix, flags::FixMode::Apply(fix_suggested) | flags::FixMode::Diff(fix_suggested)) {
if let Ok(FixerResult {
result,
transformed,
Expand All @@ -405,6 +420,7 @@ pub(crate) fn lint_stdin(
&settings.linter,
&source_kind,
source_type,
fix_suggested
) {
match autofix {
flags::FixMode::Apply => {
Expand All @@ -429,13 +445,48 @@ pub(crate) fn lint_stdin(
stdout.write_all(b"\n")?;
stdout.flush()?;
}
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(_) => {}
}
flags::FixMode::Generate => {}
}

(result, fixed)
} else {
// If we fail to autofix, lint the original source code.
(result, fixed)
} else {
// If we fail to autofix, lint the original source code.
let result = lint_only(
contents,
path.unwrap_or_else(|| Path::new("-")),
package,
settings,
noqa,
Some(&source_kind),
source_type,
);
let fixed = FxHashMap::default();

// Write the contents to stdout anyway.
if autofix.is_apply() {
io::stdout().write_all(contents.as_bytes())?;
}

(result, fixed)
}
}
flags::FixMode::Generate(_) => {
let result = lint_only(
path.unwrap_or_else(|| Path::new("-")),
package,
Expand Down
26 changes: 19 additions & 7 deletions crates/ruff_cli/src/lib.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ use log::warn;
use notify::{recommended_watcher, RecursiveMode, Watcher};

use ruff_linter::logging::{set_up_logging, LogLevel};
use ruff_linter::settings::flags;
use ruff_linter::settings::flags::{FixMode, SuggestedFixes};
use ruff_linter::settings::types::SerializationFormat;
use ruff_linter::{fs, warn_user, warn_user_once};
use ruff_workspace::Settings;
Expand Down Expand Up @@ -228,6 +228,7 @@ pub fn check(args: CheckCommand, log_level: LogLevel) -> Result<ExitStatus> {
let Settings {
fix,
fix_only,
fix_suggested,
output_format,
show_fixes,
show_source,
Expand All @@ -237,15 +238,26 @@ pub fn check(args: CheckCommand, log_level: LogLevel) -> Result<ExitStatus> {
// 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).
// print them to stdout, if we're reading from stdin) for [`Applicability::Automatic`] rules
// only.
// - If `--diff` or `--fix-only` are set, don't print any violations (only
// fixes).
// fixes) for [`Applicability::Automatic`] rules only.
// - If `--fix--suggested` is set, the above rules will apply to both [`Applicability::Suggested`] and
// [`Applicability::Automatic`] fixes.
// TODO: can't fix this until @zanieb changes the CLI
let fix_suggested = if fix_suggested {
SuggestedFixes::Apply
} else {
SuggestedFixes::Disable
};

let autofix = if cli.diff {
flags::FixMode::Diff
FixMode::Diff(fix_suggested)
} else if fix || fix_only {
flags::FixMode::Apply
FixMode::Apply(fix_suggested)
} else {
flags::FixMode::Generate
// We'll always generate all fixes, regardless of [`Applicability`], in `generate` mode
FixMode::Generate(SuggestedFixes::Apply)
};
let cache = !cli.no_cache;
let noqa = !cli.ignore_noqa;
Expand Down Expand Up @@ -382,7 +394,7 @@ pub fn check(args: CheckCommand, log_level: LogLevel) -> Result<ExitStatus> {
// Always try to print violations (the printer itself may suppress output),
// unless we're writing fixes via stdin (in which case, the transformed
// source code goes to stdout).
if !(is_stdin && matches!(autofix, flags::FixMode::Apply | flags::FixMode::Diff)) {
if !(is_stdin && matches!(autofix, FixMode::Apply(_) | FixMode::Diff(_))) {
if cli.statistics {
printer.write_statistics(&diagnostics, &mut writer)?;
} else {
Expand Down
Loading

0 comments on commit ec2edd0

Please sign in to comment.