From bf7925df7b1aac0265e3bf88ef8ca05d720e0560 Mon Sep 17 00:00:00 2001 From: Michael Suo Date: Mon, 2 May 2022 14:36:59 -0700 Subject: [PATCH] feat: format command Add a `format` command: - linters can be identified as formatters with the `is_formatter` config option - `lintrunner format` will run formatting lints and accept all changes - add infer_subcommands so you can do `lintrunner f`. --- src/lib.rs | 14 +-- src/lint_config.rs | 13 +- src/main.rs | 115 ++++++++++++------ tests/integration_test.rs | 41 +++++++ ...t_command_doesnt_use_nonformat_linter.snap | 13 ++ 5 files changed, 148 insertions(+), 48 deletions(-) create mode 100644 tests/snapshots/integration_test__format_command_doesnt_use_nonformat_linter.snap diff --git a/src/lib.rs b/src/lib.rs index 0911e4a..49dbd0d 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -121,7 +121,7 @@ fn get_paths_from_file(file: AbsPath) -> Result> { } /// Represents the set of paths the user wants to lint. -pub enum PathsToLint { +pub enum PathsOpt { /// The user didn't specify any paths, so we'll automatically determine /// which paths to check. Auto, @@ -150,7 +150,7 @@ pub enum RenderOpt { pub fn do_lint( linters: Vec, - paths_to_lint: PathsToLint, + paths_opt: PathsOpt, should_apply_patches: bool, render_opt: RenderOpt, enable_spinners: bool, @@ -161,8 +161,8 @@ pub fn do_lint( linters.iter().map(|l| &l.code).collect::>() ); - let mut files = match paths_to_lint { - PathsToLint::Auto => { + let mut files = match paths_opt { + PathsOpt::Auto => { let git_root = get_git_root()?; let relative_to = match revision_opt { RevisionOpt::Head => None, @@ -173,9 +173,9 @@ pub fn do_lint( }; get_changed_files(&git_root, relative_to.as_deref())? } - PathsToLint::PathsCmd(paths_cmd) => get_paths_from_cmd(paths_cmd)?, - PathsToLint::Paths(paths) => get_paths_from_input(paths)?, - PathsToLint::PathsFile(file) => get_paths_from_file(file)?, + PathsOpt::PathsCmd(paths_cmd) => get_paths_from_cmd(paths_cmd)?, + PathsOpt::Paths(paths) => get_paths_from_input(paths)?, + PathsOpt::PathsFile(file) => get_paths_from_file(file)?, }; // Sort and unique the files so we pass a consistent ordering to linters diff --git a/src/lint_config.rs b/src/lint_config.rs index bbd6138..90b80db 100644 --- a/src/lint_config.rs +++ b/src/lint_config.rs @@ -12,6 +12,10 @@ pub struct LintRunnerConfig { pub linters: Vec, } +fn is_false(b: &bool) -> bool { + return *b == false; +} + /// Represents a single linter, along with all the information necessary to invoke it. /// /// This goes in the linter configuration TOML file. @@ -30,7 +34,7 @@ pub struct LintRunnerConfig { /// '@{{PATHSFILE}}' /// ] /// ``` -#[derive(Serialize, Deserialize)] +#[derive(Serialize, Deserialize, Clone)] pub struct LintConfig { /// The name of the linter, conventionally capitals and numbers, no spaces, /// dashes, or underscores @@ -103,6 +107,13 @@ pub struct LintConfig { /// ```toml /// command = ['python3', 'my_linter_init.py', '--dry-run={{DRYRUN}}'] pub init_command: Option>, + + /// If true, this linter will be considered a formatter, and will invoked by + /// `lintrunner format`. Formatters should be *safe*: people should be able + /// to blindly accept the output without worrying that it will change the + /// meaning of their code. + #[serde(skip_serializing_if = "is_false", default = "bool::default")] + pub is_formatter: bool, } /// Given options specified by the user, return a list of linters to run. diff --git a/src/main.rs b/src/main.rs index 2663b1b..190133d 100644 --- a/src/main.rs +++ b/src/main.rs @@ -10,38 +10,38 @@ use lintrunner::{ path::AbsPath, persistent_data::PersistentDataStore, render::print_error, - PathsToLint, RenderOpt, RevisionOpt, + PathsOpt, RenderOpt, RevisionOpt, }; #[derive(Debug, Parser)] -#[structopt(name = "lintrunner", about = "A lint runner")] +#[clap(name = "lintrunner", about = "A lint runner", infer_subcommands(true))] struct Args { /// Verbose mode (-v, or -vv to show full list of paths being linted) - #[clap(short, long, parse(from_occurrences))] + #[clap(short, long, parse(from_occurrences), global = true)] verbose: u8, /// Path to a toml file defining which linters to run - #[clap(long, default_value = ".lintrunner.toml")] + #[clap(long, default_value = ".lintrunner.toml", global = true)] config: String, /// If set, any suggested patches will be applied - #[clap(short, long)] + #[clap(short, long, global = true)] apply_patches: bool, /// Shell command that returns new-line separated paths to lint /// /// Example: To run on all files in the repo, use `--paths-cmd='git grep -Il .'`. - #[clap(long, conflicts_with = "paths-from")] + #[clap(long, conflicts_with = "paths-from", global = true)] paths_cmd: Option, /// File with new-line separated paths to lint - #[clap(long)] + #[clap(long, global = true)] paths_from: Option, /// Lint all files that differ between the working directory and the /// specified revision. This argument can be any that is accepted /// by `git diff-tree` - #[clap(long, short, conflicts_with_all=&["paths", "paths-cmd", "paths-from"])] + #[clap(long, short, conflicts_with_all=&["paths", "paths-cmd", "paths-from"], global = true)] revision: Option, /// Lint all files that differ between the merge base of HEAD with the @@ -49,41 +49,41 @@ struct Args { /// accepted by `git diff-tree` /// /// Example: lintrunner -m master - #[clap(long, short, conflicts_with_all=&["paths", "paths-cmd", "paths-from", "revision"])] + #[clap(long, short, conflicts_with_all=&["paths", "paths-cmd", "paths-from", "revision"], global = true)] merge_base_with: Option, /// Comma-separated list of linters to skip (e.g. --skip CLANGFORMAT,NOQA) - #[clap(long)] + #[clap(long, global = true)] skip: Option, /// Comma-separated list of linters to run (opposite of --skip) - #[clap(long)] + #[clap(long, global = true)] take: Option, /// With 'default' show lint issues in human-readable format, for interactive use. /// With 'json', show lint issues as machine-readable JSON (one per line) /// With 'oneline', show lint issues in compact format (one per line) - #[clap(long, arg_enum, default_value_t = RenderOpt::Default)] + #[clap(long, arg_enum, default_value_t = RenderOpt::Default, global=true)] output: RenderOpt, + /// Paths to lint. lintrunner will still respect the inclusions and #[clap(subcommand)] cmd: Option, - /// Paths to lint. lintrunner will still respect the inclusions and /// exclusions defined in .lintrunner.toml; manually specifying a path will /// not override them. - #[clap(conflicts_with_all = &["paths-cmd", "paths-from"])] + #[clap(conflicts_with_all = &["paths-cmd", "paths-from"], global = true)] paths: Vec, /// If set, always output with ANSI colors, even if we detect the output is /// not a user-attended terminal. - #[clap(long)] + #[clap(long, global = true)] force_color: bool, /// If set, use ths provided path to store any metadata generated by /// lintrunner. By default, this is a platform-specific location for /// application data (e.g. $XDG_DATA_HOME for UNIX systems.) - #[clap(long)] + #[clap(long, global = true)] data_path: Option, } @@ -95,6 +95,12 @@ enum SubCommand { #[clap(long, short)] dry_run: bool, }, + /// Run and accept changes for formatting linters only. Equivalent to + /// `lintrunner --apply-patches --take `. + Format, + + /// Run linters. This is the default if no subcommand is provided. + Lint, } fn do_main() -> Result { @@ -120,6 +126,10 @@ fn do_main() -> Result { let config_path = AbsPath::try_from(&args.config) .with_context(|| format!("Could not read lintrunner config at: '{}'", args.config))?; + + let cmd = args.cmd.unwrap_or(SubCommand::Lint); + let lint_runner_config = LintRunnerConfig::new(&config_path)?; + let skipped_linters = args.skip.map(|linters| { linters .split(',') @@ -133,28 +143,32 @@ fn do_main() -> Result { .collect::>() }); - let lint_runner_config = LintRunnerConfig::new(&config_path)?; + // If we are formatting, the universe of linters to select from should be + // restricted to only formatters. + // (NOTE: we pay an allocation for `placeholder` even in cases where we are + // just passing through a reference in the else-branch. This doesn't matter, + // but if we want to fix it we should impl Cow for LintConfig and use that + // instead.). + let mut placeholder = Vec::new(); + let all_linters = if let SubCommand::Format = &cmd { + let iter = lint_runner_config + .linters + .iter() + .filter(|l| l.is_formatter) + .map(|l| l.clone()); + placeholder.extend(iter); + &placeholder + } else { + // If we're not formatting, all linters defined in the config are + // eligible to run. + &lint_runner_config.linters + }; - let linters = get_linters_from_config( - &lint_runner_config.linters, - skipped_linters, - taken_linters, - &config_path, - )?; + let linters = + get_linters_from_config(all_linters, skipped_linters, taken_linters, &config_path)?; let enable_spinners = args.verbose == 0 && args.output == RenderOpt::Default; - - let paths_to_lint = if let Some(paths_file) = args.paths_from { - let path_file = AbsPath::try_from(&paths_file) - .with_context(|| format!("Failed to find `--paths-from` file '{}'", paths_file))?; - PathsToLint::PathsFile(path_file) - } else if let Some(paths_cmd) = args.paths_cmd { - PathsToLint::PathsCmd(paths_cmd) - } else if !args.paths.is_empty() { - PathsToLint::Paths(args.paths) - } else { - PathsToLint::Auto - }; + let persistent_data_store = PersistentDataStore::new(&config_path)?; let revision_opt = if let Some(revision) = args.revision { RevisionOpt::Revision(revision) @@ -164,19 +178,40 @@ fn do_main() -> Result { RevisionOpt::Head }; - let persistent_data_store = PersistentDataStore::new(&config_path)?; + let paths_opt = if let Some(paths_file) = args.paths_from { + let path_file = AbsPath::try_from(&paths_file) + .with_context(|| format!("Failed to find `--paths-from` file '{}'", paths_file))?; + PathsOpt::PathsFile(path_file) + } else if let Some(paths_cmd) = args.paths_cmd { + PathsOpt::PathsCmd(paths_cmd) + } else if !args.paths.is_empty() { + PathsOpt::Paths(args.paths) + } else { + PathsOpt::Auto + }; - match args.cmd { - Some(SubCommand::Init { dry_run }) => { + match cmd { + SubCommand::Init { dry_run } => { // Just run initialization commands, don't actually lint. do_init(linters, dry_run, &persistent_data_store, &config_path) } - None => { + SubCommand::Format => { + check_init_changed(&persistent_data_store, &lint_runner_config)?; + do_lint( + linters, + paths_opt, + true, // always apply patches when we use the format command + args.output, + enable_spinners, + revision_opt, + ) + } + SubCommand::Lint => { // Default command is to just lint. check_init_changed(&persistent_data_store, &lint_runner_config)?; do_lint( linters, - paths_to_lint, + paths_opt, args.apply_patches, args.output, enable_spinners, diff --git a/tests/integration_test.rs b/tests/integration_test.rs index d44df4a..c8634ed 100644 --- a/tests/integration_test.rs +++ b/tests/integration_test.rs @@ -562,3 +562,44 @@ fn excluding_dryrun_fails() -> Result<()> { Ok(()) } + +#[test] +fn format_command_doesnt_use_nonformat_linter() -> Result<()> { + let data_path = tempfile::tempdir()?; + let lint_message = LintMessage { + path: Some("tests/fixtures/fake_source_file.rs".to_string()), + line: Some(9), + char: Some(1), + code: "DUMMY".to_string(), + name: "dummy failure".to_string(), + severity: LintSeverity::Advice, + original: None, + replacement: None, + description: Some("A dummy linter failure".to_string()), + }; + let config = temp_config(&format!( + "\ + [[linter]] + code = 'TESTLINTER' + include_patterns = ['**'] + command = ['echo', '{}'] + ", + serde_json::to_string(&lint_message)? + ))?; + + let mut cmd = Command::cargo_bin("lintrunner")?; + cmd.arg(format!("--config={}", config.path().to_str().unwrap())); + cmd.arg(format!( + "--data-path={}", + data_path.path().to_str().unwrap() + )); + + // Run on a file to ensure that the linter is run. + cmd.arg("format"); + cmd.arg("README.md"); + // Should succeed because TESTLINTER was not run + cmd.assert().success(); + assert_output_snapshot("format_command_doesnt_use_nonformat_linter", &mut cmd)?; + + Ok(()) +} diff --git a/tests/snapshots/integration_test__format_command_doesnt_use_nonformat_linter.snap b/tests/snapshots/integration_test__format_command_doesnt_use_nonformat_linter.snap new file mode 100644 index 0000000..5202735 --- /dev/null +++ b/tests/snapshots/integration_test__format_command_doesnt_use_nonformat_linter.snap @@ -0,0 +1,13 @@ +--- +source: tests/integration_test.rs +assertion_line: 20 +expression: output_lines +--- +- "STDOUT:" +- ok No lint issues. +- Successfully applied all patches. +- "" +- "" +- "STDERR:" +- "WARNING: No previous init data found. If this is the first time you're running lintrunner, you should run `lintrunner init`." +