From 2bc55ba8158d2ed20d4a4f609bc2afc220bfb8ce Mon Sep 17 00:00:00 2001
From: Casey Rodarmor <casey@rodarmor.com>
Date: Fri, 11 Nov 2016 14:33:17 -0800
Subject: [PATCH] Improve a few error messages (#47)

Surround variables with backticks, capitalize first letter of error
message, inflect properly depending on number of unknown overrides, and
improve wording.

Also added build dependency to `filter` recipe.
---
 justfile           |  2 +-
 src/integration.rs | 68 ++++++++++++++++++++++++++++++++++++++++++++--
 src/lib.rs         | 39 +++++++++++++++++++-------
 3 files changed, 96 insertions(+), 13 deletions(-)

diff --git a/justfile b/justfile
index f9a6122..f35cd9e 100644
--- a/justfile
+++ b/justfile
@@ -2,7 +2,7 @@ test: build
 	cargo test --lib
 
 # only run tests matching PATTERN
-filter PATTERN:
+filter PATTERN: build
 	cargo test --lib {{PATTERN}}
 
 test-quine:
diff --git a/src/integration.rs b/src/integration.rs
index b463888..b189b91 100644
--- a/src/integration.rs
+++ b/src/integration.rs
@@ -434,7 +434,7 @@ fn unknown_override_options() {
 a = `exit 222`",
     255,
     "",
-    "baz and foo set on the command line but not present in justfile\n",
+    "Variables `baz` and `foo` overridden on the command line but not present in justfile\n",
   );
 }
 
@@ -448,7 +448,21 @@ fn unknown_override_args() {
 a = `exit 222`",
     255,
     "",
-    "baz and foo set on the command line but not present in justfile\n",
+    "Variables `baz` and `foo` overridden on the command line but not present in justfile\n",
+  );
+}
+
+#[test]
+fn unknown_override_arg() {
+  integration_test(
+    &["foo=bar", "a=b", "a", "b"],
+    "foo:
+ echo hello
+ echo {{`exit 111`}}
+a = `exit 222`",
+    255,
+    "",
+    "Variable `foo` overridden on the command line but not present in justfile\n",
   );
 }
 
@@ -746,3 +760,53 @@ foo A B:
     "echo A:ONE B:TWO\n",
   );
 }
+
+#[test]
+fn argument_mismatch_more() {
+  integration_test(
+    &["foo", "ONE", "TWO", "THREE"],
+    "
+foo A B:
+  echo A:{{A}} B:{{B}}
+    ",
+    255,
+    "",
+    "Recipe `foo` got 3 arguments but only takes 2\n"
+  );
+}
+
+#[test]
+fn argument_mismatch_fewer() {
+  integration_test(
+    &["foo", "ONE"],
+    "
+foo A B:
+  echo A:{{A}} B:{{B}}
+    ",
+    255,
+    "",
+    "Recipe `foo` got 1 argument but takes 2\n"
+  );
+}
+
+#[test]
+fn unknown_recipe() {
+  integration_test(
+    &["foo"],
+    "hello:",
+    255,
+    "",
+    "Justfile does not contain recipe `foo`\n",
+  );
+}
+
+#[test]
+fn unknown_recipes() {
+  integration_test(
+    &["foo", "bar"],
+    "hello:",
+    255,
+    "",
+    "Justfile does not contain recipes `foo` or `bar`\n",
+  );
+}
diff --git a/src/lib.rs b/src/lib.rs
index 188ee42..7d8a774 100644
--- a/src/lib.rs
+++ b/src/lib.rs
@@ -708,6 +708,26 @@ fn mixed_whitespace(text: &str) -> bool {
   !(text.chars().all(|c| c == ' ') || text.chars().all(|c| c == '\t'))
 }
 
+fn maybe_s(n: usize) -> &'static str {
+  if n == 1 {
+    ""
+  } else {
+    "s"
+  }
+}
+
+struct Tick<'a, T: 'a + Display>(&'a T);
+
+impl<'a, T: Display> Display for Tick<'a, T> {
+  fn fmt(&self, f: &mut fmt::Formatter) -> Result<(), fmt::Error> {
+    write!(f, "`{}`", self.0)
+  }
+}
+
+fn ticks<'a, T: 'a + Display>(ts: &'a [T]) -> Vec<Tick<'a, T>> {
+  ts.iter().map(Tick).collect()
+}
+
 struct And<'a, T: 'a + Display>(&'a [T]);
 struct Or <'a, T: 'a + Display>(&'a [T]);
 
@@ -1008,23 +1028,22 @@ impl<'a> Display for RunError<'a> {
   fn fmt(&self, f: &mut fmt::Formatter) -> Result<(), fmt::Error> {
     match *self {
       RunError::UnknownRecipes{ref recipes} => {
-        if recipes.len() == 1 { 
-          try!(write!(f, "Justfile does not contain recipe `{}`", recipes[0]));
-        } else {
-          try!(write!(f, "Justfile does not contain recipes: {}", recipes.join(" ")));
-        };
+        try!(write!(f, "Justfile does not contain recipe{} {}",
+                    maybe_s(recipes.len()), Or(&ticks(&recipes))));
       },
       RunError::UnknownOverrides{ref overrides} => {
-        try!(write!(f, "{} set on the command line but not present in justfile",
-                    And(overrides)))
+        try!(write!(f, 
+                    "Variable{} {} overridden on the command line but not present in justfile",
+                    maybe_s(overrides.len()),
+                    And(&overrides.iter().map(Tick).collect::<Vec<_>>())))
       },
       RunError::NonLeadingRecipeWithParameters{recipe} => {
         try!(write!(f, "Recipe `{}` takes arguments and so must be the first and only recipe specified on the command line", recipe));
       },
       RunError::ArgumentCountMismatch{recipe, found, expected} => {
-        try!(write!(f, "Recipe `{}` takes {} argument{}, but {}{} were found",
-                    recipe, expected, if expected == 1 { "" } else { "s" }, 
-                    if found < expected { "only " } else { "" }, found));
+        try!(write!(f, "Recipe `{}` got {} argument{} but {}takes {}",
+                    recipe, found, maybe_s(found),
+                    if expected < found { "only " } else { "" }, expected));
       },
       RunError::Code{recipe, code} => {
         try!(write!(f, "Recipe \"{}\" failed with exit code {}", recipe, code));