Repository: suo/lintrunner (language: python)

Issue:
feat: Allow multiple toml files in config
Allow multiple toml files in config


This pr allows the usage of multiple configuration files with lintrunner. If a conflict occurs the latter file in the order they are entered takes precedence.

This allows us to have custom actions for lintrunner internally, and for users of lintrunner personal configurations if they have weird branch names or something of that manner.

---
Stack created with [Sapling](https://sapling-scm.com). Best reviewed with [ReviewStack](https://reviewstack.dev/suo/lintrunner/pull/56).
* #60
* #57
* __->__ #56


Discussion / hints:

Fix patch:
diff --git a/Cargo.toml b/Cargo.toml
index 6fc3d86..71494a1 100644
--- a/Cargo.toml
+++ b/Cargo.toml
@@ -27,6 +27,7 @@ fern = { version = "0.6.1", features = ["colored"] }
 chrono = "0.4.19"
 dialoguer = "0.10.1"
 shell-words = "1.1.0"
+figment = { version = "0.10", features = ["toml", "env"] }
 
 [dev-dependencies]
 assert_cmd = "2.0.4"
diff --git a/README.md b/README.md
index 5d3d17e..035c76f 100644
--- a/README.md
+++ b/README.md
@@ -43,6 +43,13 @@ every path in the repo is:
 git grep -Il . | xargs lintrunner
 ```
 
+### `--configs`/ `--config`
+"Comma-separated paths to lintrunner configuration files. 
+Multiple files are merged, with later definitions overriding earlier ones. 
+ONLY THE FIRST is required to be present on your machine. 
+Defaults to `lintrunner.toml, lintrunner.private.toml`. Extra configs like `lintrunner.private.toml`
+ are useful for combining project-wide and local configs."
+
 ### `--paths-cmd`
 Some ways to invoke `xargs` will cause multiple `lintrunner` processes to be
 run, increasing lint time (especially on huge path sets). As an alternative that
diff --git a/src/init.rs b/src/init.rs
index 49f9d43..595eb49 100644
--- a/src/init.rs
+++ b/src/init.rs
@@ -24,7 +24,7 @@ pub fn check_init_changed(
         return Ok(());
     }
     let last_init = last_init.unwrap();
-    let old_config = LintRunnerConfig::new_from_string(&last_init)?;
+    let old_config: LintRunnerConfig = serde_json::from_str(&last_init)?;
 
     let old_init_commands: Vec<_> = old_config.linters.iter().map(|l| &l.init_command).collect();
     let current_init_commands: Vec<_> = current_config
diff --git a/src/lib.rs b/src/lib.rs
index a26ba57..5c9904b 100644
--- a/src/lib.rs
+++ b/src/lib.rs
@@ -75,7 +75,7 @@ pub fn do_init(
     linters: Vec<Linter>,
     dry_run: bool,
     persistent_data_store: &PersistentDataStore,
-    config_path: &AbsPath,
+    config_paths: &Vec<std::string::String>,
 ) -> Result<i32> {
     debug!(
         "Initializing linters: {:?}",
@@ -85,9 +85,7 @@ pub fn do_init(
     for linter in linters {
         linter.init(dry_run)?;
     }
-
-    persistent_data_store.update_last_init(config_path)?;
-
+    persistent_data_store.update_last_init(config_paths)?;
     Ok(0)
 }
 
diff --git a/src/lint_config.rs b/src/lint_config.rs
index b1fc6d5..7e38d5e 100644
--- a/src/lint_config.rs
+++ b/src/lint_config.rs
@@ -2,6 +2,10 @@ use std::{collections::HashSet, fs};
 
 use crate::{linter::Linter, path::AbsPath};
 use anyhow::{bail, ensure, Context, Result};
+use figment::{
+    providers::{Format, Toml},
+    Figment,
+};
 use glob::Pattern;
 use log::debug;
 use serde::{Deserialize, Serialize};
@@ -123,11 +127,11 @@ pub struct LintConfig {
 }
 
 /// Given options specified by the user, return a list of linters to run.
-pub fn get_linters_from_config(
+pub fn get_linters_from_configs(
     linter_configs: &[LintConfig],
     skipped_linters: Option<HashSet<String>>,
     taken_linters: Option<HashSet<String>>,
-    config_path: &AbsPath,
+    primary_config_path: &AbsPath,
 ) -> Result<Vec<Linter>> {
     let mut linters = Vec::new();
     let mut all_linters: HashSet<String> = HashSet::new();
@@ -160,7 +164,7 @@ pub fn get_linters_from_config(
             exclude_patterns,
             commands: lint_config.command.clone(),
             init_commands: lint_config.init_command.clone(),
-            config_path: config_path.clone(),
+            primary_config_path: primary_config_path.clone(),
         });
     }
 
@@ -198,15 +202,23 @@ pub fn get_linters_from_config(
 }
 
 impl LintRunnerConfig {
-    pub fn new(path: &AbsPath) -> Result<LintRunnerConfig> {
-        let lint_config = fs::read_to_string(&path)
-            .context(format!("Failed to read config file: '{}'.", path.display()))?;
-        LintRunnerConfig::new_from_string(&lint_config)
-    }
+    pub fn new(paths: &Vec<std::string::String>) -> Result<LintRunnerConfig> {
+        let mut config = Figment::new();
+        for path in paths {
+            let config_str = fs::read_to_string(path)
+                .context(format!("Could not read config file at {}", path))?;
+
+            // schema check
+            let _test_str = toml::from_str::<toml::Value>(&config_str)
+                .context(format!("Config file at {} had invalid schema", path))?;
+
+            config = config.merge(Toml::file(path));
+        }
+
+        let config = config
+            .extract::<LintRunnerConfig>()
+            .context("Config file had invalid schema")?;
 
-    pub fn new_from_string(config_str: &str) -> Result<LintRunnerConfig> {
-        let config: LintRunnerConfig =
-            toml::from_str(config_str).context("Config file had invalid schema")?;
         for linter in &config.linters {
             if let Some(init_args) = &linter.init_command {
                 if init_args.iter().all(|arg| !arg.contains("{{DRYRUN}}")) {
@@ -218,7 +230,6 @@ impl LintRunnerConfig {
                 }
             }
         }
-
         Ok(config)
     }
 }
diff --git a/src/linter.rs b/src/linter.rs
index 0d8e807..fc4c16f 100644
--- a/src/linter.rs
+++ b/src/linter.rs
@@ -17,7 +17,7 @@ pub struct Linter {
     pub exclude_patterns: Vec<Pattern>,
     pub commands: Vec<String>,
     pub init_commands: Option<Vec<String>>,
-    pub config_path: AbsPath,
+    pub primary_config_path: AbsPath,
 }
 
 fn matches_relative_path(base: &Path, from: &Path, pattern: &Pattern) -> bool {
@@ -39,7 +39,7 @@ fn matches_relative_path(base: &Path, from: &Path, pattern: &Pattern) -> bool {
 impl Linter {
     fn get_config_dir(&self) -> &Path {
         // Unwrap is fine here because we know this path is absolute and won't be `/`
-        self.config_path.parent().unwrap()
+        self.primary_config_path.parent().unwrap()
     }
 
     fn get_matches(&self, files: &[AbsPath]) -> Vec<AbsPath> {
@@ -180,6 +180,7 @@ impl Linter {
                     .iter()
                     .map(|arg| arg.replace("{{DRYRUN}}", dry_run))
                     .collect();
+                info!("the init commands are {:?}", init_commands);
                 let (program, arguments) = init_commands.split_at(1);
                 debug!(
                     "Running: {} {}",
@@ -194,6 +195,7 @@ impl Linter {
                     .args(arguments)
                     .current_dir(self.get_config_dir())
                     .status()?;
+                info!("the status is {:?}", status);
                 ensure!(
                     status.success(),
                     "lint initializer for '{}' failed with non-zero exit code",
diff --git a/src/main.rs b/src/main.rs
index 59eb27c..ec1aba6 100644
--- a/src/main.rs
+++ b/src/main.rs
@@ -1,13 +1,14 @@
-use std::{collections::HashSet, convert::TryFrom, io::Write};
+use std::{collections::HashSet, convert::TryFrom, io::Write, path::Path};
 
 use anyhow::{Context, Result};
 use chrono::SecondsFormat;
 use clap::Parser;
 
+use itertools::Itertools;
 use lintrunner::{
     do_init, do_lint,
     init::check_init_changed,
-    lint_config::{get_linters_from_config, LintRunnerConfig},
+    lint_config::{get_linters_from_configs, LintRunnerConfig},
     log_utils::setup_logger,
     path::AbsPath,
     persistent_data::{ExitInfo, PersistentDataStore, RunInfo},
@@ -26,9 +27,17 @@ struct Args {
     #[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", global = true)]
-    config: String,
+    /// Paths to TOML files specifying linters. Configs are merged, with later files overriding earlier ones.
+    /// Except for the first, all files are optional, with missing ones triggering a warning.
+    /// Relative paths are interpreted with respect to the first config file.
+    #[clap(
+        long,
+        global = true,
+        alias = "config",
+        multiple = true,
+        default_value = ".lintrunner.toml, .lintrunner.private.toml"
+    )]
+    configs: String,
 
     /// If set, any suggested patches will be applied
     #[clap(short, long, global = true)]
@@ -127,9 +136,6 @@ enum SubCommand {
 fn do_main() -> Result<i32> {
     let args = Args::parse();
 
-    let config_path = AbsPath::try_from(&args.config)
-        .with_context(|| format!("Could not read lintrunner config at: '{}'", args.config))?;
-
     if args.force_color {
         console::set_colors_enabled(true);
         console::set_colors_enabled_stderr(true);
@@ -152,7 +158,17 @@ fn do_main() -> Result<i32> {
         args: std::env::args().collect(),
         timestamp: chrono::Local::now().to_rfc3339_opts(SecondsFormat::Millis, true),
     };
-    let persistent_data_store = PersistentDataStore::new(&config_path, run_info)?;
+    // clone split by commas and trim whitespace
+    let config_paths: Vec<String> = args
+        .configs
+        .split(",")
+        .map(|path| path.trim().to_string())
+        .collect_vec();
+    // check if first config path exists
+    let primary_config_path = AbsPath::try_from(config_paths[0].clone())
+        .with_context(|| format!("Could not read lintrunner config at: '{}'", config_paths[0]))?;
+
+    let persistent_data_store = PersistentDataStore::new(&primary_config_path, run_info)?;
 
     setup_logger(
         log_level,
@@ -166,9 +182,20 @@ fn do_main() -> Result<i32> {
     let repo = version_control::Repo::new()?;
     debug!("Current rev: {}", repo.get_head()?);
 
-    let cmd = args.cmd.unwrap_or(SubCommand::Lint);
-    let lint_runner_config = LintRunnerConfig::new(&config_path)?;
+    // report config paths which do not exist
+    for path in &config_paths {
+        match AbsPath::try_from(path) {
+            Ok(_) => {},  // do nothing on success
+            Err(_) => eprintln!("Warning: Could not find a lintrunner config at: '{}'. Continuing without using configuration file.", path),
+        }
+    }
 
+    let config_paths: Vec<String> = config_paths
+        .into_iter()
+        .filter(|path| Path::new(&path).exists())
+        .collect();
+    let cmd = args.cmd.unwrap_or(SubCommand::Lint);
+    let lint_runner_config = LintRunnerConfig::new(&config_paths)?;
     let skipped_linters = args.skip.map(|linters| {
         linters
             .split(',')
@@ -203,8 +230,12 @@ fn do_main() -> Result<i32> {
         &lint_runner_config.linters
     };
 
-    let linters =
-        get_linters_from_config(all_linters, skipped_linters, taken_linters, &config_path)?;
+    let linters = get_linters_from_configs(
+        all_linters,
+        skipped_linters,
+        taken_linters,
+        &primary_config_path,
+    )?;
 
     let enable_spinners = args.verbose == 0 && args.output == RenderOpt::Default;
 
@@ -240,7 +271,7 @@ fn do_main() -> Result<i32> {
     let res = match cmd {
         SubCommand::Init { dry_run } => {
             // Just run initialization commands, don't actually lint.
-            do_init(linters, dry_run, &persistent_data_store, &config_path)
+            do_init(linters, dry_run, &persistent_data_store, &config_paths)
         }
         SubCommand::Format => {
             check_init_changed(&persistent_data_store, &lint_runner_config)?;
diff --git a/src/persistent_data.rs b/src/persistent_data.rs
index 63d0d22..cd22413 100644
--- a/src/persistent_data.rs
+++ b/src/persistent_data.rs
@@ -8,6 +8,7 @@
 
 use anyhow::{anyhow, bail, Context, Result};
 use directories::ProjectDirs;
+use figment::providers::{Format, Toml};
 use log::debug;
 use serde::{Deserialize, Serialize};
 use std::{
@@ -15,7 +16,7 @@ use std::{
     path::{Path, PathBuf},
 };
 
-use crate::path::AbsPath;
+use crate::{lint_config::LintRunnerConfig, path::AbsPath};
 
 const CONFIG_DATA_NAME: &str = ".lintrunner.toml";
 const RUNS_DIR_NAME: &str = "runs";
@@ -52,7 +53,10 @@ impl RunInfo {
 }
 
 impl PersistentDataStore {
-    pub fn new(config_path: &AbsPath, cur_run_info: RunInfo) -> Result<PersistentDataStore> {
+    pub fn new(
+        primary_config_path: &AbsPath,
+        cur_run_info: RunInfo,
+    ) -> Result<PersistentDataStore> {
         // Retrieve the lintrunner-wide data directory.
         let project_dirs = ProjectDirs::from("", "", "lintrunner");
         let project_dirs =
@@ -60,7 +64,8 @@ impl PersistentDataStore {
         let project_data_dir = project_dirs.data_dir();
 
         // Now compute one specific to this lintrunner config.
-        let config_path_hash = blake3::hash(config_path.to_string_lossy().as_bytes()).to_string();
+        let config_path_hash =
+            blake3::hash(primary_config_path.to_string_lossy().as_bytes()).to_string();
         let config_data_dir = project_data_dir.join(config_path_hash);
 
         // Create the runs dir as well.
@@ -231,17 +236,23 @@ impl PersistentDataStore {
         Ok(Some(std::fs::read_to_string(init_path)?))
     }
 
-    pub fn update_last_init(&self, config_path: &AbsPath) -> Result<()> {
+    pub fn update_last_init(&self, config_paths: &Vec<std::string::String>) -> Result<()> {
         debug!(
-            "Writing used config to {}/{}",
+            "Writing used config(s) to {}/{}",
             self.data_dir.display(),
             CONFIG_DATA_NAME
         );
 
-        let config_contents = std::fs::read_to_string(config_path)?;
-        let path = self.relative_path(CONFIG_DATA_NAME);
+        let mut config_contents = figment::Figment::new();
+
+        for path in config_paths {
+            config_contents = config_contents.join(Toml::file(path));
+        }
 
-        std::fs::write(path, &config_contents)?;
+        let config_contents = config_contents.extract::<LintRunnerConfig>()?;
+        let path = self.relative_path(CONFIG_DATA_NAME);
+        let serialized_contents = serde_json::to_string_pretty(&config_contents)?;
+        std::fs::write(path, &serialized_contents)?;
         Ok(())
     }
 


Test patch:
diff --git a/tests/integration_test.rs b/tests/integration_test.rs
index 5b5e9a9..97ef06f 100644
--- a/tests/integration_test.rs
+++ b/tests/integration_test.rs
@@ -156,6 +156,88 @@ fn simple_linter() -> Result<()> {
     Ok(())
 }
 
+#[test]
+#[cfg_attr(target_os = "windows", ignore)] // STDERR string is different
+fn simple_linter_two_configs() -> Result<()> {
+    let data_path = tempfile::tempdir()?;
+    let lint_message1 = 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 lint_message2 = LintMessage {
+        path: Some("tests/fixtures/fake_source_file.rs".to_string()),
+        line: Some(9),
+        char: Some(1),
+        code: "DUMMY".to_string(),
+        name: "real dummy failure".to_string(),
+        severity: LintSeverity::Advice,
+        original: None,
+        replacement: None,
+        description: Some("The real dummy linter failure".to_string()),
+    };
+    let config1 = temp_config_returning_msg(lint_message1)?;
+    let config2 = temp_config_returning_msg(lint_message2)?;
+
+    let mut cmd = Command::cargo_bin("lintrunner")?;
+    cmd.arg(format!(
+        "--config={},{}",
+        config1.path().to_str().unwrap(),
+        config2.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("README.md");
+    cmd.assert().failure();
+    assert_output_snapshot("simple_linter_two_configs", &mut cmd)?;
+
+    Ok(())
+}
+
+#[test]
+#[cfg_attr(target_os = "windows", ignore)] // STDERR string is different
+fn simple_linter_succeeds_with_nonexistent_second_config() -> 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_returning_msg(lint_message)?;
+
+    let mut cmd = Command::cargo_bin("lintrunner")?;
+    cmd.arg(format!(
+        "--config={},{}",
+        config.path().to_str().unwrap(),
+        "NONEXISTENT_CONFIG"
+    ));
+    cmd.arg(format!(
+        "--data-path={}",
+        data_path.path().to_str().unwrap()
+    ));
+    // Run on a file to ensure that the linter is run.
+    cmd.arg("README.md");
+    cmd.assert().failure();
+    assert_output_snapshot("simple_linter_fake_second_config", &mut cmd)?;
+
+    Ok(())
+}
+
 #[test]
 #[cfg_attr(target_os = "windows", ignore)] // path is rendered differently
 fn simple_linter_oneline() -> Result<()> {
@@ -536,6 +618,9 @@ fn changed_init_causes_warning() -> Result<()> {
         data_path.path().to_str().unwrap()
     ));
     cmd.arg("init");
+    println!("the commadn is {:?}", cmd);
+    println!("the the assert is {:?}", cmd.assert());
+    println!("the result of the command is {:?}", cmd.output()?);
     cmd.assert().success();
 
     let mut cmd = Command::cargo_bin("lintrunner")?;
diff --git a/tests/snapshots/integration_test__invalid_config_fails.snap b/tests/snapshots/integration_test__invalid_config_fails.snap
index b41a3b9..429a753 100644
--- a/tests/snapshots/integration_test__invalid_config_fails.snap
+++ b/tests/snapshots/integration_test__invalid_config_fails.snap
@@ -1,12 +1,11 @@
 ---
 source: tests/integration_test.rs
 expression: output_lines
-
 ---
 - "STDOUT:"
 - ""
 - ""
 - "STDERR:"
 - "error:        Config file had invalid schema"
-- "caused_by:             missing field `linter` at line 1 column 1"
+- "caused_by:             missing field `linter`"
 
diff --git a/tests/snapshots/integration_test__invalid_paths_cmd_and_from.snap b/tests/snapshots/integration_test__invalid_paths_cmd_and_from.snap
index 8b786a2..6766016 100644
--- a/tests/snapshots/integration_test__invalid_paths_cmd_and_from.snap
+++ b/tests/snapshots/integration_test__invalid_paths_cmd_and_from.snap
@@ -1,7 +1,6 @@
 ---
 source: tests/integration_test.rs
 expression: output_lines
-
 ---
 - "STDOUT:"
 - ""
@@ -10,7 +9,7 @@ expression: output_lines
 - "error: The argument '--paths-cmd <PATHS_CMD>' cannot be used with '--paths-from <PATHS_FROM>'"
 - ""
 - "USAGE:"
-- "    lintrunner --config <CONFIG> --paths-cmd <PATHS_CMD>"
+- "    lintrunner --configs <CONFIGS>... --paths-cmd <PATHS_CMD>"
 - ""
 - For more information try --help
 
diff --git a/tests/snapshots/integration_test__invalid_paths_cmd_and_specified_paths.snap b/tests/snapshots/integration_test__invalid_paths_cmd_and_specified_paths.snap
index fcf4415..764eaf9 100644
--- a/tests/snapshots/integration_test__invalid_paths_cmd_and_specified_paths.snap
+++ b/tests/snapshots/integration_test__invalid_paths_cmd_and_specified_paths.snap
@@ -1,7 +1,6 @@
 ---
 source: tests/integration_test.rs
 expression: output_lines
-
 ---
 - "STDOUT:"
 - ""
@@ -10,7 +9,7 @@ expression: output_lines
 - "error: The argument '--paths-cmd <PATHS_CMD>' cannot be used with '<PATHS>...'"
 - ""
 - "USAGE:"
-- "    lintrunner --config <CONFIG> --paths-cmd <PATHS_CMD>"
+- "    lintrunner --configs <CONFIGS>... --paths-cmd <PATHS_CMD>"
 - ""
 - For more information try --help
 
diff --git a/tests/snapshots/integration_test__simple_linter_fake_second_config.snap b/tests/snapshots/integration_test__simple_linter_fake_second_config.snap
new file mode 100644
index 0000000..f850ad6
--- /dev/null
+++ b/tests/snapshots/integration_test__simple_linter_fake_second_config.snap
@@ -0,0 +1,26 @@
+---
+source: tests/integration_test.rs
+expression: output_lines
+---
+- "STDOUT:"
+- ""
+- ""
+- ">>> Lint for tests/fixtures/fake_source_file.rs:"
+- ""
+- "  Advice (DUMMY) dummy failure"
+- "    A dummy linter failure"
+- ""
+- "         6  |use std::io::Write;"
+- "         7  |"
+- "         8  |fn assert_output_snapshot(cmd: &mut Command) -> Result<()> {"
+- "    >>>  9  |    let re = Regex::new(\"<temp-config>\").unwrap();"
+- "        10  |    let output = cmd.output()?;"
+- "        11  |"
+- "        12  |    let output_string = format!("
+- ""
+- ""
+- ""
+- "STDERR:"
+- "Warning: Could not find a lintrunner config at: 'NONEXISTENT_CONFIG'. Continuing without using configuration file."
+- "WARNING: No previous init data found. If this is the first time you're running lintrunner, you should run `lintrunner init`."
+
diff --git a/tests/snapshots/integration_test__simple_linter_two_configs.snap b/tests/snapshots/integration_test__simple_linter_two_configs.snap
new file mode 100644
index 0000000..5382929
--- /dev/null
+++ b/tests/snapshots/integration_test__simple_linter_two_configs.snap
@@ -0,0 +1,25 @@
+---
+source: tests/integration_test.rs
+expression: output_lines
+---
+- "STDOUT:"
+- ""
+- ""
+- ">>> Lint for tests/fixtures/fake_source_file.rs:"
+- ""
+- "  Advice (DUMMY) real dummy failure"
+- "    The real dummy linter failure"
+- ""
+- "         6  |use std::io::Write;"
+- "         7  |"
+- "         8  |fn assert_output_snapshot(cmd: &mut Command) -> Result<()> {"
+- "    >>>  9  |    let re = Regex::new(\"<temp-config>\").unwrap();"
+- "        10  |    let output = cmd.output()?;"
+- "        11  |"
+- "        12  |    let output_string = format!("
+- ""
+- ""
+- ""
+- "STDERR:"
+- "WARNING: No previous init data found. If this is the first time you're running lintrunner, you should run `lintrunner init`."
+