Refactor run::run and Config (#490)

Improve color parsing, add `ConfigError`, put `invocation_directory` on Config object, return error code from `run::run` instead of exiting.

src/assignment_evaluator.rs

@@ -167,14 +167,10 @@ mod test {
   use super::*;
   use crate::testing::parse;
 
-  fn no_cwd_err() -> Result<PathBuf, String> {
-    Err(String::from("no cwd in tests"))
-  }
-
   #[test]
   fn backtick_code() {
     match parse("a:\n echo {{`f() { return 100; }; f`}}")
-      .run(&no_cwd_err(), &["a"], &Default::default())
+      .run(&["a"], &Default::default())
       .unwrap_err()
     {
       RuntimeError::Backtick {
@@ -202,10 +198,7 @@ recipe:
       ..Default::default()
     };
 
-    match parse(text)
+    match parse(text).run(&["recipe"], &config).unwrap_err() {
-      .run(&no_cwd_err(), &["recipe"], &config)
-      .unwrap_err()
-    {
       RuntimeError::Backtick {
         token,
         output_error: OutputError::Code(_),

src/common.rs

@@ -3,7 +3,9 @@ pub(crate) use std::{
   borrow::Cow,
   cmp,
   collections::{BTreeMap, BTreeSet},
+  convert::AsRef,
   env,
+  ffi::OsStr,
   fmt::{self, Display, Formatter},
   fs, io, iter,
   ops::{Range, RangeInclusive},
@@ -16,7 +18,7 @@ pub(crate) use std::{
 
 // dependencies
 pub(crate) use edit_distance::edit_distance;
-pub(crate) use libc::{EXIT_FAILURE, EXIT_SUCCESS};
+pub(crate) use libc::EXIT_FAILURE;
 pub(crate) use log::warn;
 pub(crate) use unicode_width::UnicodeWidthChar;
 
@@ -38,21 +40,23 @@ pub(crate) use crate::{
 pub(crate) use crate::{
   alias::Alias, alias_resolver::AliasResolver, assignment_evaluator::AssignmentEvaluator,
   assignment_resolver::AssignmentResolver, color::Color, compilation_error::CompilationError,
-  compilation_error_kind::CompilationErrorKind, config::Config, expression::Expression,
+  compilation_error_kind::CompilationErrorKind, config::Config, config_error::ConfigError,
-  fragment::Fragment, function::Function, function_context::FunctionContext, functions::Functions,
+  expression::Expression, fragment::Fragment, function::Function,
-  interrupt_guard::InterruptGuard, interrupt_handler::InterruptHandler, justfile::Justfile,
+  function_context::FunctionContext, functions::Functions, interrupt_guard::InterruptGuard,
-  lexer::Lexer, output_error::OutputError, parameter::Parameter, parser::Parser,
+  interrupt_handler::InterruptHandler, justfile::Justfile, lexer::Lexer, output_error::OutputError,
-  platform::Platform, position::Position, recipe::Recipe, recipe_context::RecipeContext,
+  parameter::Parameter, parser::Parser, platform::Platform, position::Position, recipe::Recipe,
-  recipe_resolver::RecipeResolver, runtime_error::RuntimeError, search_error::SearchError,
+  recipe_context::RecipeContext, recipe_resolver::RecipeResolver, runtime_error::RuntimeError,
-  shebang::Shebang, state::State, string_literal::StringLiteral, subcommand::Subcommand,
+  search_error::SearchError, shebang::Shebang, state::State, string_literal::StringLiteral,
-  token::Token, token_kind::TokenKind, use_color::UseColor, variables::Variables,
+  subcommand::Subcommand, token::Token, token_kind::TokenKind, use_color::UseColor,
-  verbosity::Verbosity, warning::Warning,
+  variables::Variables, verbosity::Verbosity, warning::Warning,
 };
 
 pub(crate) type CompilationResult<'a, T> = Result<T, CompilationError<'a>>;
 
 pub(crate) type RunResult<'a, T> = Result<T, RuntimeError<'a>>;
 
+pub(crate) type ConfigResult<T> = Result<T, ConfigError>;
+
 #[allow(unused_imports)]
 pub(crate) use std::io::prelude::*;
 

src/config.rs

@@ -15,14 +15,24 @@ pub(crate) struct Config<'a> {
   pub(crate) color: Color,
   pub(crate) verbosity: Verbosity,
   pub(crate) arguments: Vec<&'a str>,
+  pub(crate) justfile: Option<&'a Path>,
+  pub(crate) working_directory: Option<&'a Path>,
+  pub(crate) invocation_directory: Result<PathBuf, String>,
 }
 
 mod arg {
-  pub(crate) const EDIT: &str = "EDIT";
-  pub(crate) const SUMMARY: &str = "SUMMARY";
   pub(crate) const DUMP: &str = "DUMP";
+  pub(crate) const COLOR: &str = "COLOR";
+  pub(crate) const EDIT: &str = "EDIT";
   pub(crate) const LIST: &str = "LIST";
   pub(crate) const SHOW: &str = "SHOW";
+  pub(crate) const SUMMARY: &str = "SUMMARY";
+  pub(crate) const WORKING_DIRECTORY: &str = "WORKING-DIRECTORY";
+
+  pub(crate) const COLOR_AUTO: &str = "auto";
+  pub(crate) const COLOR_ALWAYS: &str = "always";
+  pub(crate) const COLOR_NEVER: &str = "never";
+  pub(crate) const COLOR_VALUES: &[&str] = &[COLOR_AUTO, COLOR_ALWAYS, COLOR_NEVER];
 }
 
 impl<'a> Config<'a> {
@@ -38,11 +48,11 @@ impl<'a> Config<'a> {
           .help("The recipe(s) to run, defaults to the first recipe in the justfile"),
       )
       .arg(
-        Arg::with_name("COLOR")
+        Arg::with_name(arg::COLOR)
           .long("color")
           .takes_value(true)
-          .possible_values(&["auto", "always", "never"])
+          .possible_values(arg::COLOR_VALUES)
-          .default_value("auto")
+          .default_value(arg::COLOR_AUTO)
           .help("Print colorful output"),
       )
       .arg(
@@ -129,7 +139,7 @@ impl<'a> Config<'a> {
           .help("Use verbose output"),
       )
       .arg(
-        Arg::with_name("WORKING-DIRECTORY")
+        Arg::with_name(arg::WORKING_DIRECTORY)
           .short("d")
           .long("working-directory")
           .takes_value(true)
@@ -164,18 +174,28 @@ impl<'a> Config<'a> {
     }
   }
 
-  pub(crate) fn from_matches(matches: &'a ArgMatches<'a>) -> Config<'a> {
+  fn color_from_value(value: &str) -> ConfigResult<Color> {
+    match value {
+      arg::COLOR_AUTO => Ok(Color::auto()),
+      arg::COLOR_ALWAYS => Ok(Color::always()),
+      arg::COLOR_NEVER => Ok(Color::never()),
+      _ => Err(ConfigError::Internal {
+        message: format!("Invalid argument `{}` to --color.", value),
+      }),
+    }
+  }
+
+  pub(crate) fn from_matches(matches: &'a ArgMatches<'a>) -> ConfigResult<Config<'a>> {
+    let invocation_directory =
+      env::current_dir().map_err(|e| format!("Error getting current directory: {}", e));
+
     let verbosity = Verbosity::from_flag_occurrences(matches.occurrences_of("VERBOSE"));
 
-    let color = match matches.value_of("COLOR").expect("`--color` had no value") {
+    let color = Self::color_from_value(
-      "auto" => Color::auto(),
+      matches
-      "always" => Color::always(),
+        .value_of(arg::COLOR)
-      "never" => Color::never(),
+        .expect("`--color` had no value"),
-      other => die!(
+    )?;
-        "Invalid argument `{}` to --color. This is a bug in just.",
-        other
-      ),
-    };
 
     let set_count = matches.occurrences_of("SET");
     let mut overrides = BTreeMap::new();
@@ -216,7 +236,7 @@ impl<'a> Config<'a> {
       .flat_map(|(i, argument)| {
         if i == 0 {
           if let Some(i) = argument.rfind('/') {
-            if matches.is_present("WORKING-DIRECTORY") {
+            if matches.is_present(arg::WORKING_DIRECTORY) {
               die!("--working-directory and a path prefixed recipe may not be used together.");
             }
 
@@ -252,18 +272,21 @@ impl<'a> Config<'a> {
       Subcommand::Run
     };
 
-    Config {
+    Ok(Config {
       dry_run: matches.is_present("DRY-RUN"),
       evaluate: matches.is_present("EVALUATE"),
       highlight: matches.is_present("HIGHLIGHT"),
       quiet: matches.is_present("QUIET"),
       shell: matches.value_of("SHELL").unwrap(),
+      justfile: matches.value_of("JUSTFILE").map(Path::new),
+      working_directory: matches.value_of("WORKING-DIRECTORY").map(Path::new),
+      invocation_directory,
       subcommand,
       verbosity,
       color,
       overrides,
       arguments,
-    }
+    })
   }
 }
 
@@ -280,6 +303,10 @@ impl<'a> Default for Config<'a> {
       shell: DEFAULT_SHELL,
       color: default(),
       verbosity: Verbosity::from_flag_occurrences(0),
+      justfile: None,
+      working_directory: None,
+      invocation_directory: env::current_dir()
+        .map_err(|e| format!("Error getting current directory: {}", e)),
     }
   }
 }

src/config_error.rs

@@ -0,0 +1,20 @@
+use crate::common::*;
+
+pub(crate) enum ConfigError {
+  Internal { message: String },
+}
+
+impl Display for ConfigError {
+  fn fmt(&self, f: &mut Formatter) -> fmt::Result {
+    use ConfigError::*;
+
+    match self {
+      Internal { message } => write!(
+        f,
+        "Internal config error, this may indicate a bug in just: {} \
+         consider filing an issue: https://github.com/casey/just/issues/new",
+        message
+      ),
+    }
+  }
+}

src/justfile.rs

@@ -43,12 +43,7 @@ impl<'a> Justfile<'a> {
     None
   }
 
-  pub(crate) fn run(
+  pub(crate) fn run(&'a self, arguments: &[&'a str], config: &'a Config<'a>) -> RunResult<'a, ()> {
-    &'a self,
-    invocation_directory: &'a Result<PathBuf, String>,
-    arguments: &[&'a str],
-    config: &'a Config<'a>,
-  ) -> RunResult<'a, ()> {
     let unknown_overrides = config
       .overrides
       .keys()
@@ -66,7 +61,7 @@ impl<'a> Justfile<'a> {
 
     let scope = AssignmentEvaluator::evaluate_assignments(
       &self.assignments,
-      invocation_directory,
+      &config.invocation_directory,
       &dotenv,
       &config.overrides,
       config.quiet,
@@ -127,11 +122,7 @@ impl<'a> Justfile<'a> {
       });
     }
 
-    let context = RecipeContext {
+    let context = RecipeContext { config, scope };
-      invocation_directory,
-      config,
-      scope,
-    };
 
     let mut ran = empty();
     for (recipe, arguments) in grouped {
@@ -212,14 +203,10 @@ mod test {
   use crate::runtime_error::RuntimeError::*;
   use crate::testing::parse;
 
-  fn no_cwd_err() -> Result<PathBuf, String> {
-    Err(String::from("no cwd in tests"))
-  }
-
   #[test]
   fn unknown_recipes() {
     match parse("a:\nb:\nc:")
-      .run(&no_cwd_err(), &["a", "x", "y", "z"], &Default::default())
+      .run(&["a", "x", "y", "z"], &Default::default())
       .unwrap_err()
     {
       UnknownRecipes {
@@ -251,10 +238,7 @@ a:
       x
 ";
 
-    match parse(text)
+    match parse(text).run(&["a"], &Default::default()).unwrap_err() {
-      .run(&no_cwd_err(), &["a"], &Default::default())
-      .unwrap_err()
-    {
       Code {
         recipe,
         line_number,
@@ -271,7 +255,7 @@ a:
   #[test]
   fn code_error() {
     match parse("fail:\n @exit 100")
-      .run(&no_cwd_err(), &["fail"], &Default::default())
+      .run(&["fail"], &Default::default())
       .unwrap_err()
     {
       Code {
@@ -294,7 +278,7 @@ a return code:
  @x() { {{return}} {{code + "0"}}; }; x"#;
 
     match parse(text)
-      .run(&no_cwd_err(), &["a", "return", "15"], &Default::default())
+      .run(&["a", "return", "15"], &Default::default())
       .unwrap_err()
     {
       Code {
@@ -313,7 +297,7 @@ a return code:
   #[test]
   fn missing_some_arguments() {
     match parse("a b c d:")
-      .run(&no_cwd_err(), &["a", "b", "c"], &Default::default())
+      .run(&["a", "b", "c"], &Default::default())
       .unwrap_err()
     {
       ArgumentCountMismatch {
@@ -337,7 +321,7 @@ a return code:
   #[test]
   fn missing_some_arguments_variadic() {
     match parse("a b c +d:")
-      .run(&no_cwd_err(), &["a", "B", "C"], &Default::default())
+      .run(&["a", "B", "C"], &Default::default())
       .unwrap_err()
     {
       ArgumentCountMismatch {
@@ -361,7 +345,7 @@ a return code:
   #[test]
   fn missing_all_arguments() {
     match parse("a b c d:\n echo {{b}}{{c}}{{d}}")
-      .run(&no_cwd_err(), &["a"], &Default::default())
+      .run(&["a"], &Default::default())
       .unwrap_err()
     {
       ArgumentCountMismatch {
@@ -385,7 +369,7 @@ a return code:
   #[test]
   fn missing_some_defaults() {
     match parse("a b c d='hello':")
-      .run(&no_cwd_err(), &["a", "b"], &Default::default())
+      .run(&["a", "b"], &Default::default())
       .unwrap_err()
     {
       ArgumentCountMismatch {
@@ -409,7 +393,7 @@ a return code:
   #[test]
   fn missing_all_defaults() {
     match parse("a b c='r' d='h':")
-      .run(&no_cwd_err(), &["a"], &Default::default())
+      .run(&["a"], &Default::default())
       .unwrap_err()
     {
       ArgumentCountMismatch {
@@ -436,7 +420,7 @@ a return code:
     config.overrides.insert("foo", "bar");
     config.overrides.insert("baz", "bob");
     match parse("a:\n echo {{`f() { return 100; }; f`}}")
-      .run(&no_cwd_err(), &["a"], &config)
+      .run(&["a"], &config)
       .unwrap_err()
     {
       UnknownOverrides { overrides } => {
@@ -463,10 +447,7 @@ wut:
       ..Default::default()
     };
 
-    match parse(text)
+    match parse(text).run(&["wut"], &config).unwrap_err() {
-      .run(&no_cwd_err(), &["wut"], &config)
-      .unwrap_err()
-    {
       Code {
         code: _,
         line_number,

src/lib.rs

@@ -21,6 +21,7 @@ mod common;
 mod compilation_error;
 mod compilation_error_kind;
 mod config;
+mod config_error;
 mod expression;
 mod fragment;
 mod function;

src/main.rs

@@ -1,3 +1,5 @@
 fn main() {
-  just::run();
+  if let Err(code) = just::run() {
+    std::process::exit(code);
+  }
 }

src/recipe.rs

@@ -82,7 +82,7 @@ impl<'a> Recipe<'a> {
       assignments: &empty(),
       dry_run: config.dry_run,
       evaluated: empty(),
-      invocation_directory: context.invocation_directory,
+      invocation_directory: &config.invocation_directory,
       overrides: &empty(),
       quiet: config.quiet,
       scope: &context.scope,

src/recipe_context.rs

@@ -1,7 +1,6 @@
 use crate::common::*;
 
 pub(crate) struct RecipeContext<'a> {
-  pub(crate) invocation_directory: &'a Result<PathBuf, String>,
   pub(crate) config: &'a Config<'a>,
   pub(crate) scope: BTreeMap<&'a str, String>,
 }

src/run.rs

@@ -1,27 +1,38 @@
 use crate::common::*;
 
 use crate::{interrupt_handler::InterruptHandler, misc::maybe_s};
-use std::{convert, ffi};
 use unicode_width::UnicodeWidthStr;
 
-#[cfg(windows)]
+fn edit<P: AsRef<OsStr>>(path: P) -> Result<(), i32> {
-use ansi_term::enable_ansi_support;
+  let editor = match env::var_os("EDITOR") {
-
+    None => {
-fn edit<P: convert::AsRef<ffi::OsStr>>(path: P) -> ! {
+      eprintln!("Error getting EDITOR environment variable");
-  let editor =
+      return Err(EXIT_FAILURE);
-    env::var_os("EDITOR").unwrap_or_else(|| die!("Error getting EDITOR environment variable"));
+    }
+    Some(editor) => editor,
+  };
 
   let error = Command::new(editor).arg(path).status();
 
   match error {
-    Ok(status) => process::exit(status.code().unwrap_or(EXIT_FAILURE)),
+    Ok(status) => {
-    Err(error) => die!("Failed to invoke editor: {}", error),
+      if status.success() {
+        Ok(())
+      } else {
+        eprintln!("Editor failed: {}", status);
+        Err(status.code().unwrap_or(EXIT_FAILURE))
+      }
+    }
+    Err(error) => {
+      eprintln!("Failed to invoke editor: {}", error);
+      Err(EXIT_FAILURE)
+    }
   }
 }
 
-pub fn run() {
+pub fn run() -> Result<(), i32> {
   #[cfg(windows)]
-  enable_ansi_support().ok();
+  ansi_term::enable_ansi_support().ok();
 
   env_logger::Builder::from_env(
     env_logger::Env::new()
@@ -30,18 +41,21 @@ pub fn run() {
   )
   .init();
 
-  let invocation_directory =
-    env::current_dir().map_err(|e| format!("Error getting current directory: {}", e));
-
   let app = Config::app();
 
   let matches = app.get_matches();
 
-  let config = Config::from_matches(&matches);
+  let config = match Config::from_matches(&matches) {
+    Ok(config) => config,
+    Err(error) => {
+      eprintln!("error: {}", error);
+      return Err(EXIT_FAILURE);
+    }
+  };
 
-  let justfile = matches.value_of("JUSTFILE").map(Path::new);
+  let justfile = config.justfile;
 
-  let mut working_directory = matches.value_of("WORKING-DIRECTORY").map(PathBuf::from);
+  let mut working_directory = config.working_directory.map(PathBuf::from);
 
   if let (Some(justfile), None) = (justfile, working_directory.as_ref()) {
     let mut justfile = justfile.to_path_buf();
@@ -49,11 +63,14 @@ pub fn run() {
     if !justfile.is_absolute() {
       match justfile.canonicalize() {
         Ok(canonical) => justfile = canonical,
-        Err(err) => die!(
+        Err(err) => {
+          eprintln!(
             "Could not canonicalize justfile path `{}`: {}",
             justfile.display(),
             err
-        ),
+          );
+          return Err(EXIT_FAILURE);
+        }
       }
     }
 
@@ -65,7 +82,7 @@ pub fn run() {
   let text;
   if let (Some(justfile), Some(directory)) = (justfile, working_directory) {
     if config.subcommand == Subcommand::Edit {
-      edit(justfile);
+      return edit(justfile);
     }
 
     text = fs::read_to_string(justfile)
@@ -86,32 +103,45 @@ pub fn run() {
     match search::justfile(&current_dir) {
       Ok(name) => {
         if config.subcommand == Subcommand::Edit {
-          edit(name);
+          return edit(name);
         }
-        text = fs::read_to_string(&name)
+        text = match fs::read_to_string(&name) {
-          .unwrap_or_else(|error| die!("Error reading justfile: {}", error));
+          Err(error) => {
+            eprintln!("Error reading justfile: {}", error);
+            return Err(EXIT_FAILURE);
+          }
+          Ok(text) => text,
+        };
 
         let parent = name.parent().unwrap();
 
         if let Err(error) = env::set_current_dir(&parent) {
-          die!(
+          eprintln!(
             "Error changing directory to {}: {}",
             parent.display(),
             error
           );
+          return Err(EXIT_FAILURE);
         }
       }
-      Err(search_error) => die!("{}", search_error),
+      Err(search_error) => {
+        eprintln!("{}", search_error);
+        return Err(EXIT_FAILURE);
+      }
     }
   }
 
-  let justfile = Parser::parse(&text).unwrap_or_else(|error| {
+  let justfile = match Parser::parse(&text) {
+    Err(error) => {
       if config.color.stderr().active() {
-      die!("{:#}", error);
+        eprintln!("{:#}", error);
       } else {
-      die!("{}", error);
+        eprintln!("{}", error);
       }
-  });
+      return Err(EXIT_FAILURE);
+    }
+    Ok(justfile) => justfile,
+  };
 
   for warning in &justfile.warnings {
     if config.color.stderr().active() {
@@ -135,12 +165,12 @@ pub fn run() {
         .join(" ");
       println!("{}", summary);
     }
-    process::exit(EXIT_SUCCESS);
+    return Ok(());
   }
 
   if config.subcommand == Subcommand::Dump {
     println!("{}", justfile);
-    process::exit(EXIT_SUCCESS);
+    return Ok(());
   }
 
   if config.subcommand == Subcommand::List {
@@ -227,7 +257,7 @@ pub fn run() {
       }
     }
 
-    process::exit(EXIT_SUCCESS);
+    return Ok(());
   }
 
   if let Subcommand::Show { name } = config.subcommand {
@@ -235,17 +265,17 @@ pub fn run() {
       let recipe = justfile.get_recipe(alias.target).unwrap();
       println!("{}", alias);
       println!("{}", recipe);
-      process::exit(EXIT_SUCCESS);
+      return Ok(());
     }
     if let Some(recipe) = justfile.get_recipe(name) {
       println!("{}", recipe);
-      process::exit(EXIT_SUCCESS);
+      return Ok(());
     } else {
       eprintln!("Justfile does not contain recipe `{}`.", name);
       if let Some(suggestion) = justfile.suggest(name) {
         eprintln!("Did you mean `{}`?", suggestion);
       }
-      process::exit(EXIT_FAILURE)
+      return Err(EXIT_FAILURE);
     }
   }
 
@@ -270,7 +300,7 @@ pub fn run() {
     warn!("Failed to set CTRL-C handler: {}", error)
   }
 
-  if let Err(run_error) = justfile.run(&invocation_directory, &arguments, &config) {
+  if let Err(run_error) = justfile.run(&arguments, &config) {
     if !config.quiet {
       if config.color.stderr().active() {
         eprintln!("{:#}", run_error);
@@ -279,6 +309,8 @@ pub fn run() {
       }
     }
 
-    process::exit(run_error.code().unwrap_or(EXIT_FAILURE));
+    return Err(run_error.code().unwrap_or(EXIT_FAILURE));
   }
+
+  Ok(())
 }

src/runtime_error.rs

@@ -377,7 +377,7 @@ impl<'a> Display for RuntimeError<'a> {
       Internal { ref message } => {
         write!(
           f,
-          "Internal error, this may indicate a bug in just: {} \
+          "Internal runtime error, this may indicate a bug in just: {} \
            consider filing an issue: https://github.com/casey/just/issues/new",
           message
         )?;