Move subcommand execution into Subcommand (#514)

Moves the code which executes subcommands into Subcommand:run,
delegating to separate functions for each subcommand. This reduces the
disgustingness of `run::run` a bit, and paves the way for future
refactoring and cleanup.

src/common.rs

@@ -3,9 +3,7 @@ pub(crate) use std::{
   borrow::Cow,
   cmp,
   collections::{BTreeMap, BTreeSet},
-  convert::AsRef,
   env,
-  ffi::OsStr,
   fmt::{self, Display, Formatter},
   fs,
   io::{self, Write},

src/config.rs

@@ -291,7 +291,7 @@ impl<'a> Config<'a> {
     } else if let Some(name) = matches.value_of(cmd::SHOW) {
       Subcommand::Show { name }
     } else {
-      Subcommand::Run
+      Subcommand::Execute
     };
 
     Ok(Config {
@@ -315,7 +315,7 @@ impl<'a> Config<'a> {
 impl<'a> Default for Config<'a> {
   fn default() -> Config<'static> {
     Config {
-      subcommand: Subcommand::Run,
+      subcommand: Subcommand::Execute,
       dry_run: false,
       highlight: false,
       overrides: empty(),

src/run.rs

@@ -1,35 +1,5 @@
 use crate::common::*;
 
-use crate::interrupt_handler::InterruptHandler;
-use unicode_width::UnicodeWidthStr;
-
-fn edit<P: AsRef<OsStr>>(path: P) -> Result<(), i32> {
-  let editor = match env::var_os("EDITOR") {
-    None => {
-      eprintln!("Error getting EDITOR environment variable");
-      return Err(EXIT_FAILURE);
-    }
-    Some(editor) => editor,
-  };
-
-  let error = Command::new(editor).arg(path).status();
-
-  match error {
-    Ok(status) => {
-      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() -> Result<(), i32> {
   #[cfg(windows)]
   ansi_term::enable_ansi_support().ok();
@@ -92,7 +62,7 @@ pub fn run() -> Result<(), i32> {
   let text;
   if let (Some(justfile), Some(directory)) = (justfile, working_directory) {
     if config.subcommand == Subcommand::Edit {
-      return edit(justfile);
+      return Subcommand::edit(justfile);
     }
 
     text = fs::read_to_string(justfile)
@@ -113,7 +83,7 @@ pub fn run() -> Result<(), i32> {
     match search::justfile(&current_dir) {
       Ok(name) => {
         if config.subcommand == Subcommand::Edit {
-          return edit(name);
+          return Subcommand::edit(&name);
         }
         text = match fs::read_to_string(&name) {
           Err(error) => {
@@ -161,166 +131,5 @@ pub fn run() -> Result<(), i32> {
     }
   }
 
-  if config.subcommand == Subcommand::Summary {
+  config.subcommand.run(&config, justfile)
-    if justfile.count() == 0 {
-      eprintln!("Justfile contains no recipes.");
-    } else {
-      let summary = justfile
-        .recipes
-        .iter()
-        .filter(|&(_, recipe)| !recipe.private)
-        .map(|(name, _)| name)
-        .cloned()
-        .collect::<Vec<_>>()
-        .join(" ");
-      println!("{}", summary);
-    }
-    return Ok(());
-  }
-
-  if config.subcommand == Subcommand::Dump {
-    println!("{}", justfile);
-    return Ok(());
-  }
-
-  if config.subcommand == Subcommand::List {
-    // Construct a target to alias map.
-    let mut recipe_aliases: BTreeMap<&str, Vec<&str>> = BTreeMap::new();
-    for alias in justfile.aliases.values() {
-      if alias.is_private() {
-        continue;
-      }
-
-      if !recipe_aliases.contains_key(alias.target.lexeme()) {
-        recipe_aliases.insert(alias.target.lexeme(), vec![alias.name.lexeme()]);
-      } else {
-        let aliases = recipe_aliases.get_mut(alias.target.lexeme()).unwrap();
-        aliases.push(alias.name.lexeme());
-      }
-    }
-
-    let mut line_widths: BTreeMap<&str, usize> = BTreeMap::new();
-
-    for (name, recipe) in &justfile.recipes {
-      if recipe.private {
-        continue;
-      }
-
-      for name in iter::once(name).chain(recipe_aliases.get(name).unwrap_or(&Vec::new())) {
-        let mut line_width = UnicodeWidthStr::width(*name);
-
-        for parameter in &recipe.parameters {
-          line_width += UnicodeWidthStr::width(format!(" {}", parameter).as_str());
-        }
-
-        if line_width <= 30 {
-          line_widths.insert(name, line_width);
-        }
-      }
-    }
-
-    let max_line_width = cmp::min(line_widths.values().cloned().max().unwrap_or(0), 30);
-
-    let doc_color = config.color.stdout().doc();
-    println!("Available recipes:");
-
-    for (name, recipe) in &justfile.recipes {
-      if recipe.private {
-        continue;
-      }
-
-      let alias_doc = format!("alias for `{}`", recipe.name);
-
-      for (i, name) in iter::once(name)
-        .chain(recipe_aliases.get(name).unwrap_or(&Vec::new()))
-        .enumerate()
-      {
-        print!("    {}", name);
-        for parameter in &recipe.parameters {
-          if config.color.stdout().active() {
-            print!(" {:#}", parameter);
-          } else {
-            print!(" {}", parameter);
-          }
-        }
-
-        // Declaring this outside of the nested loops will probably be more efficient, but
-        // it creates all sorts of lifetime issues with variables inside the loops.
-        // If this is inlined like the docs say, it shouldn't make any difference.
-        let print_doc = |doc| {
-          print!(
-            " {:padding$}{} {}",
-            "",
-            doc_color.paint("#"),
-            doc_color.paint(doc),
-            padding = max_line_width
-              .saturating_sub(line_widths.get(name).cloned().unwrap_or(max_line_width))
-          );
-        };
-
-        match (i, recipe.doc) {
-          (0, Some(doc)) => print_doc(doc),
-          (0, None) => (),
-          _ => print_doc(&alias_doc),
-        }
-        println!();
-      }
-    }
-
-    return Ok(());
-  }
-
-  if let Subcommand::Show { name } = config.subcommand {
-    if let Some(alias) = justfile.get_alias(name) {
-      let recipe = justfile.get_recipe(alias.target.lexeme()).unwrap();
-      println!("{}", alias);
-      println!("{}", recipe);
-      return Ok(());
-    }
-    if let Some(recipe) = justfile.get_recipe(name) {
-      println!("{}", recipe);
-      return Ok(());
-    } else {
-      eprintln!("Justfile does not contain recipe `{}`.", name);
-      if let Some(suggestion) = justfile.suggest(name) {
-        eprintln!("Did you mean `{}`?", suggestion);
-      }
-      return Err(EXIT_FAILURE);
-    }
-  }
-
-  let arguments = if !config.arguments.is_empty() {
-    config.arguments.clone()
-  } else if let Some(recipe) = justfile.first() {
-    let min_arguments = recipe.min_arguments();
-    if min_arguments > 0 {
-      die!(
-        "Recipe `{}` cannot be used as default recipe since it requires at least {} {}.",
-        recipe.name,
-        min_arguments,
-        Count("argument", min_arguments),
-      );
-    }
-    vec![recipe.name()]
-  } else {
-    die!("Justfile contains no recipes.");
-  };
-
-  if let Err(error) = InterruptHandler::install() {
-    warn!("Failed to set CTRL-C handler: {}", error)
-  }
-
-  if let Err(run_error) = justfile.run(&arguments, &config) {
-    if !config.quiet {
-      if config.color.stderr().active() {
-        eprintln!("{:#}", run_error);
-      } else {
-        eprintln!("{}", run_error);
-      }
-    }
-
-    return Err(run_error.code().unwrap_or(EXIT_FAILURE));
-  }
-
-  Ok(())
 }

src/subcommand.rs

@@ -1,10 +1,224 @@
-#[derive(PartialEq)]
+use crate::common::*;
+
+use unicode_width::UnicodeWidthStr;
+
+#[derive(PartialEq, Clone, Copy)]
 pub(crate) enum Subcommand<'a> {
   Dump,
   Edit,
   Evaluate,
+  Execute,
   List,
-  Run,
   Show { name: &'a str },
   Summary,
 }
+
+impl<'a> Subcommand<'a> {
+  pub(crate) fn run(self, config: &Config, justfile: Justfile) -> Result<(), i32> {
+    use Subcommand::*;
+
+    match self {
+      Dump => Self::dump(justfile),
+      Edit => {
+        eprintln!("Internal error: Subcommand::run unexpectadly invoked on Edit variant!");
+        Err(EXIT_FAILURE)
+      }
+      Execute | Evaluate => Self::execute(config, justfile),
+      List => Self::list(config, justfile),
+      Show { name } => Self::show(justfile, name),
+      Summary => Self::summary(justfile),
+    }
+  }
+
+  fn dump(justfile: Justfile) -> Result<(), i32> {
+    println!("{}", justfile);
+    Ok(())
+  }
+
+  pub(crate) fn edit(path: &Path) -> Result<(), i32> {
+    let editor = match env::var_os("EDITOR") {
+      None => {
+        eprintln!("Error getting EDITOR environment variable");
+        return Err(EXIT_FAILURE);
+      }
+      Some(editor) => editor,
+    };
+
+    let error = Command::new(editor).arg(path).status();
+
+    match error {
+      Ok(status) => {
+        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)
+      }
+    }
+  }
+
+  fn list(config: &Config, justfile: Justfile) -> Result<(), i32> {
+    // Construct a target to alias map.
+    let mut recipe_aliases: BTreeMap<&str, Vec<&str>> = BTreeMap::new();
+    for alias in justfile.aliases.values() {
+      if alias.is_private() {
+        continue;
+      }
+
+      if !recipe_aliases.contains_key(alias.target.lexeme()) {
+        recipe_aliases.insert(alias.target.lexeme(), vec![alias.name.lexeme()]);
+      } else {
+        let aliases = recipe_aliases.get_mut(alias.target.lexeme()).unwrap();
+        aliases.push(alias.name.lexeme());
+      }
+    }
+
+    let mut line_widths: BTreeMap<&str, usize> = BTreeMap::new();
+
+    for (name, recipe) in &justfile.recipes {
+      if recipe.private {
+        continue;
+      }
+
+      for name in iter::once(name).chain(recipe_aliases.get(name).unwrap_or(&Vec::new())) {
+        let mut line_width = UnicodeWidthStr::width(*name);
+
+        for parameter in &recipe.parameters {
+          line_width += UnicodeWidthStr::width(format!(" {}", parameter).as_str());
+        }
+
+        if line_width <= 30 {
+          line_widths.insert(name, line_width);
+        }
+      }
+    }
+
+    let max_line_width = cmp::min(line_widths.values().cloned().max().unwrap_or(0), 30);
+
+    let doc_color = config.color.stdout().doc();
+    println!("Available recipes:");
+
+    for (name, recipe) in &justfile.recipes {
+      if recipe.private {
+        continue;
+      }
+
+      let alias_doc = format!("alias for `{}`", recipe.name);
+
+      for (i, name) in iter::once(name)
+        .chain(recipe_aliases.get(name).unwrap_or(&Vec::new()))
+        .enumerate()
+      {
+        print!("    {}", name);
+        for parameter in &recipe.parameters {
+          if config.color.stdout().active() {
+            print!(" {:#}", parameter);
+          } else {
+            print!(" {}", parameter);
+          }
+        }
+
+        // Declaring this outside of the nested loops will probably be more efficient, but
+        // it creates all sorts of lifetime issues with variables inside the loops.
+        // If this is inlined like the docs say, it shouldn't make any difference.
+        let print_doc = |doc| {
+          print!(
+            " {:padding$}{} {}",
+            "",
+            doc_color.paint("#"),
+            doc_color.paint(doc),
+            padding = max_line_width
+              .saturating_sub(line_widths.get(name).cloned().unwrap_or(max_line_width))
+          );
+        };
+
+        match (i, recipe.doc) {
+          (0, Some(doc)) => print_doc(doc),
+          (0, None) => (),
+          _ => print_doc(&alias_doc),
+        }
+        println!();
+      }
+    }
+
+    Ok(())
+  }
+
+  fn execute(config: &Config, justfile: Justfile) -> Result<(), i32> {
+    let arguments = if !config.arguments.is_empty() {
+      config.arguments.clone()
+    } else if let Some(recipe) = justfile.first() {
+      let min_arguments = recipe.min_arguments();
+      if min_arguments > 0 {
+        die!(
+          "Recipe `{}` cannot be used as default recipe since it requires at least {} {}.",
+          recipe.name,
+          min_arguments,
+          Count("argument", min_arguments),
+        );
+      }
+      vec![recipe.name()]
+    } else {
+      die!("Justfile contains no recipes.");
+    };
+
+    if let Err(error) = InterruptHandler::install() {
+      warn!("Failed to set CTRL-C handler: {}", error)
+    }
+
+    if let Err(run_error) = justfile.run(&arguments, &config) {
+      if !config.quiet {
+        if config.color.stderr().active() {
+          eprintln!("{:#}", run_error);
+        } else {
+          eprintln!("{}", run_error);
+        }
+      }
+
+      return Err(run_error.code().unwrap_or(EXIT_FAILURE));
+    }
+
+    Ok(())
+  }
+
+  fn show(justfile: Justfile, name: &str) -> Result<(), i32> {
+    if let Some(alias) = justfile.get_alias(name) {
+      let recipe = justfile.get_recipe(alias.target.lexeme()).unwrap();
+      println!("{}", alias);
+      println!("{}", recipe);
+      return Ok(());
+    }
+    if let Some(recipe) = justfile.get_recipe(name) {
+      println!("{}", recipe);
+      return Ok(());
+    } else {
+      eprintln!("Justfile does not contain recipe `{}`.", name);
+      if let Some(suggestion) = justfile.suggest(name) {
+        eprintln!("Did you mean `{}`?", suggestion);
+      }
+      return Err(EXIT_FAILURE);
+    }
+  }
+
+  fn summary(justfile: Justfile) -> Result<(), i32> {
+    if justfile.count() == 0 {
+      eprintln!("Justfile contains no recipes.");
+    } else {
+      let summary = justfile
+        .recipes
+        .iter()
+        .filter(|&(_, recipe)| !recipe.private)
+        .map(|(name, _)| name)
+        .cloned()
+        .collect::<Vec<_>>()
+        .join(" ");
+      println!("{}", summary);
+    }
+    Ok(())
+  }
+}