Improve completer config parsing (#3287)

Author: casey

src/arguments.rs

@@ -95,7 +95,7 @@ pub struct Arguments {
   pub(crate) complete_aliases: bool,
   #[arg(
     add = ArgValueCompleter::new(PathCompleter::file()),
-    default_value = "cygpath",
+    default_value = Self::DEFAULT_CYGPATH,
     env = "JUST_CYGPATH",
     help = "Use binary at <CYGPATH> to convert between unix and Windows paths.",
     long,
@@ -196,15 +196,15 @@ pub struct Arguments {
   )]
   pub(crate) justfile_names: Option<Vec<String>>,
   #[arg(
-    default_value = "Available recipes:\n",
+    default_value = Arguments::DEFAULT_LIST_HEADING,
     env = "JUST_LIST_HEADING",
     help = "Print <TEXT> before list",
     long,
     value_name = "TEXT"
   )]
   pub(crate) list_heading: String,
   #[arg(
-    default_value = "    ",
+    default_value = Arguments::DEFAULT_LIST_PREFIX,
     env = "JUST_LIST_PREFIX",
     help = "Print <TEXT> before each list item",
     long,
@@ -288,7 +288,7 @@ pub struct Arguments {
   #[arg(env = "JUST_TIMESTAMP", help = "Print recipe command timestamps", long)]
   pub(crate) timestamp: bool,
   #[arg(
-    default_value = "%H:%M:%S",
+    default_value = Self::DEFAULT_TIMESTAMP_FORMAT,
     env = "JUST_TIMESTAMP_FORMAT",
     help = "Timestamp format string",
     long
@@ -473,6 +473,13 @@ pub(crate) struct Subcommand {
   pub(crate) variables: bool,
 }
 
+impl Arguments {
+  pub(crate) const DEFAULT_CYGPATH: &str = "cygpath";
+  pub(crate) const DEFAULT_LIST_HEADING: &str = "Available recipes:\n";
+  pub(crate) const DEFAULT_LIST_PREFIX: &str = "    ";
+  pub(crate) const DEFAULT_TIMESTAMP_FORMAT: &str = "%H:%M:%S";
+}
+
 impl Subcommand {
   pub(crate) const HEADING: &str = "Commands";
 }

src/compiler.rs

@@ -231,7 +231,7 @@ impl Compiler {
     paths.insert(root.clone(), root.clone());
     Analyzer::analyze(
       &asts,
-      &Config::default(),
+      &Config::new().unwrap(),
       None,
       &[],
       &[],
@@ -261,7 +261,7 @@ mod tests {
 
     let justfile_a_path = tmp.path().join("justfile");
     let loader_output =
-      Compiler::compile(&Config::default(), &loader, &justfile_a_path).unwrap_err();
+      Compiler::compile(&Config::new().unwrap(), &loader, &justfile_a_path).unwrap_err();
 
     assert_matches!(loader_output, Error::CircularImport { current, import }
       if current == tmp.path().join("subdir").join("b").lexiclean() &&

src/completer.rs

@@ -91,26 +91,31 @@ impl<'run, 'src> Completer<'run, 'src> {
       .collect()
   }
 
-  fn new(current: &'run OsStr, loader: &'src Loader) -> Option<Self> {
-    Self::try_new(current.to_str()?, loader).ok()
-  }
-
-  fn try_new(current: &'run str, loader: &'src Loader) -> RunResult<'src, Self> {
+  fn config() -> Option<Config> {
     let mut args = env::args_os().collect::<Vec<OsString>>();
 
     args.drain(1..3);
 
     let matches = Arguments::command()
       .ignore_errors(true)
       .try_get_matches_from(args)
-      .map_err(|err| Error::internal(format!("failed to parse arguments: {err}")))?;
+      .ok()?;
+
+    let arguments = Arguments::from_arg_matches(&matches).ok()?;
 
-    let arguments = Arguments::from_arg_matches(&matches).unwrap();
+    Config::from_arguments(arguments).ok()
+  }
 
-    let config = Config::from_arguments(arguments).unwrap_or(Config {
-      invocation_directory: env::current_dir().context(config_error::CurrentDir)?,
-      ..Config::default()
-    });
+  fn new(current: &'run OsStr, loader: &'src Loader) -> Option<Self> {
+    Self::try_new(current.to_str()?, loader).ok()
+  }
+
+  fn try_new(current: &'run str, loader: &'src Loader) -> RunResult<'src, Self> {
+    let config = if let Some(config) = Self::config() {
+      config
+    } else {
+      Config::new()?
+    };
 
     let search = Search::search(&config)?;
 

src/config.rs

@@ -1,6 +1,6 @@
 use super::*;
 
-#[derive(Debug, Default, PartialEq)]
+#[derive(Debug, PartialEq)]
 pub(crate) struct Config {
   pub(crate) alias_style: AliasStyle,
   pub(crate) allow_missing: bool,
@@ -42,6 +42,50 @@ pub(crate) struct Config {
 }
 
 impl Config {
+  pub(crate) fn new() -> ConfigResult<Self> {
+    Ok(Self {
+      alias_style: AliasStyle::Right,
+      allow_missing: false,
+      ceiling: None,
+      check: false,
+      color: Color::default(),
+      command_color: None,
+      complete_aliases: false,
+      cygpath: Arguments::DEFAULT_CYGPATH.into(),
+      dotenv_filename: None,
+      dotenv_path: None,
+      dry_run: false,
+      explain: false,
+      groups: Vec::new(),
+      highlight: true,
+      invocation_directory: env::current_dir().context(config_error::CurrentDir)?,
+      justfile_names: None,
+      list_heading: Arguments::DEFAULT_LIST_HEADING.into(),
+      list_prefix: Arguments::DEFAULT_LIST_PREFIX.into(),
+      list_submodules: false,
+      load_dotenv: true,
+      no_aliases: false,
+      no_dependencies: false,
+      one: false,
+      overrides: BTreeMap::new(),
+      search_config: SearchConfig::FromInvocationDirectory,
+      shell: None,
+      shell_args: None,
+      shell_command: false,
+      subcommand: Subcommand::Run {
+        arguments: Vec::new(),
+      },
+      tempdir: None,
+      time: false,
+      timestamp: false,
+      timestamp_format: Arguments::DEFAULT_TIMESTAMP_FORMAT.into(),
+      unsorted: false,
+      unstable: false,
+      verbosity: Verbosity::Taciturn,
+      yes: false,
+    })
+  }
+
   fn parse_modulepath(values: &[String]) -> ConfigResult<Modulepath> {
     let path = values.iter().map(String::as_str).collect::<Vec<&str>>();
 
@@ -1179,4 +1223,12 @@ mod tests {
       assert_eq!(overrides, vec!["bar=baz"]);
     },
   }
+
+  #[test]
+  fn default_config_matches_parsed_config() {
+    assert_eq!(
+      Config::new().unwrap(),
+      Config::from_arguments(Arguments::try_parse_from(["just"]).unwrap()).unwrap()
+    );
+  }
 }

src/invocation_parser.rs

@@ -393,7 +393,7 @@ mod tests {
     fs::write(&path, "mod foo").unwrap();
     fs::create_dir(tempdir.path().join("foo")).unwrap();
     fs::write(tempdir.path().join("foo/mod.just"), "bar:").unwrap();
-    let compilation = Compiler::compile(&Config::default(), &loader, &path).unwrap();
+    let compilation = Compiler::compile(&Config::new().unwrap(), &loader, &path).unwrap();
 
     let invocations =
       InvocationParser::parse_invocations(&compilation.justfile, &["foo", "bar"]).unwrap();
@@ -411,7 +411,7 @@ mod tests {
     fs::write(&path, "mod foo").unwrap();
     fs::create_dir(tempdir.path().join("foo")).unwrap();
     fs::write(tempdir.path().join("foo/mod.just"), "bar:").unwrap();
-    let compilation = Compiler::compile(&Config::default(), &loader, &path).unwrap();
+    let compilation = Compiler::compile(&Config::new().unwrap(), &loader, &path).unwrap();
 
     assert_matches!(
       InvocationParser::parse_invocations(&compilation.justfile, &["foo", "zzz"]).unwrap_err(),
@@ -430,7 +430,7 @@ mod tests {
 
     let loader = Loader::new();
     let compilation = Compiler::compile(
-      &Config::default(),
+      &Config::new().unwrap(),
       &loader,
       &tempdir.path().join("justfile"),
     )
@@ -453,7 +453,7 @@ mod tests {
 
     let loader = Loader::new();
     let compilation = Compiler::compile(
-      &Config::default(),
+      &Config::new().unwrap(),
       &loader,
       &tempdir.path().join("justfile"),
     )
@@ -474,7 +474,7 @@ mod tests {
 
     let loader = Loader::new();
     let compilation = Compiler::compile(
-      &Config::default(),
+      &Config::new().unwrap(),
       &loader,
       &tempdir.path().join("justfile"),
     )
@@ -493,7 +493,7 @@ mod tests {
 
     let loader = Loader::new();
     let compilation = Compiler::compile(
-      &Config::default(),
+      &Config::new().unwrap(),
       &loader,
       &tempdir.path().join("justfile"),
     )
@@ -516,7 +516,7 @@ mod tests {
 
     let loader = Loader::new();
     let compilation = Compiler::compile(
-      &Config::default(),
+      &Config::new().unwrap(),
       &loader,
       &tempdir.path().join("justfile"),
     )

src/summary.rs

@@ -28,7 +28,7 @@ mod full {
 pub fn summary(path: &Path) -> io::Result<Result<Summary, String>> {
   let loader = Loader::new();
 
-  match Compiler::compile(&Config::default(), &loader, path) {
+  match Compiler::compile(&Config::new().unwrap(), &loader, path) {
     Ok(compilation) => Ok(Ok(Summary::new(&compilation.justfile))),
     Err(error) => Ok(Err(if let Error::Compile { compile_error } = error {
       compile_error.to_string()

src/testing.rs

@@ -69,7 +69,7 @@ pub(crate) fn analysis_error(
 
   match Analyzer::analyze(
     &asts,
-    &Config::default(),
+    &Config::new().unwrap(),
     None,
     &[],
     &[],

tests/completions.rs

@@ -420,3 +420,20 @@ fn usage_recipes() {
     .stdout("bar\nfoo\n")
     .success();
 }
+
+#[test]
+fn recipes_with_invalid_config() {
+  Test::new()
+    .justfile(
+      "
+        foo:
+        bar:
+      ",
+    )
+    .shell(false)
+    .env("JUST_COMPLETE", "fish")
+    .env("JUST_ALIAS_STYLE", "foo")
+    .args(complete_args(&[""]))
+    .stdout_regex("bar\nfoo\n--.*")
+    .success();
+}