From 2a1d35fbd180b7b579e956815a380cac00bd9b2d Mon Sep 17 00:00:00 2001
From: Henry Fredrick Schreiner <henry.fredrick.schreiner@cern.ch>
Date: Fri, 24 Nov 2017 14:04:20 -0500
Subject: [PATCH] Return string for error message in validators

---
 include/CLI/Option.hpp     | 20 ++++++++++------
 include/CLI/Validators.hpp | 49 +++++++++++++++++++-------------------
 tests/AppTest.cpp          |  8 +++----
 tests/HelpersTest.cpp      | 38 ++++++++++++++---------------
 tests/app_helper.hpp       |  2 +-
 5 files changed, 61 insertions(+), 56 deletions(-)

diff --git a/include/CLI/Option.hpp b/include/CLI/Option.hpp
index d0a01b71b..ef52b5a3c 100644
--- a/include/CLI/Option.hpp
+++ b/include/CLI/Option.hpp
@@ -148,7 +148,7 @@ class Option : public OptionBase<Option> {
     bool changeable_{false};
 
     /// A list of validators to run on each value parsed
-    std::vector<std::function<bool(std::string &)>> validators_;
+    std::vector<std::function<std::string(std::string &)>> validators_;
 
     /// A list of options that are required with this option
     std::set<Option *> requires_;
@@ -220,7 +220,7 @@ class Option : public OptionBase<Option> {
     }
 
     /// Adds a validator
-    Option *check(std::function<bool(const std::string &)> validator) {
+    Option *check(std::function<std::string(const std::string &)> validator) {
         validators_.emplace_back(validator);
         return this;
     }
@@ -228,8 +228,12 @@ class Option : public OptionBase<Option> {
     /// Adds a validator-like function that can change result
     Option *transform(std::function<std::string(std::string)> func) {
         validators_.push_back([func](std::string &inout) {
-            inout = func(inout);
-            return true;
+            try {
+                inout = func(inout);
+            } catch(const ValidationError &e) {
+                return std::string(e.what());
+            }
+            return std::string();
         });
         return this;
     }
@@ -430,9 +434,11 @@ class Option : public OptionBase<Option> {
         // Run the validators (can change the string)
         if(!validators_.empty()) {
             for(std::string &result : results_)
-                for(const std::function<bool(std::string &)> &vali : validators_)
-                    if(!vali(result))
-                        throw ValidationError("Failed validation: " + get_name() + "=" + result);
+                for(const std::function<std::string(std::string &)> &vali : validators_) {
+                    std::string err_msg = vali(result);
+                    if(!err_msg.empty())
+                        throw ValidationError(get_name() + ": " + err_msg);
+                }
         }
 
         bool local_result;
diff --git a/include/CLI/Validators.hpp b/include/CLI/Validators.hpp
index 263453317..7ccf034e9 100644
--- a/include/CLI/Validators.hpp
+++ b/include/CLI/Validators.hpp
@@ -4,6 +4,7 @@
 // file LICENSE or https://github.com/CLIUtils/CLI11 for details.
 
 #include "CLI/TypeTools.hpp"
+
 #include <functional>
 #include <iostream>
 #include <string>
@@ -19,64 +20,62 @@ namespace CLI {
 /// @defgroup validator_group Validators
 /// @brief Some validators that are provided
 ///
-/// These are simple `bool(std::string&)` validators that are useful.
+/// These are simple `void(std::string&)` validators that are useful. They throw
+/// a ValidationError if they fail (or the normally expected error if the cast fails)
 /// @{
 
 /// Check for an existing file
-inline bool ExistingFile(const std::string &filename) {
+inline std::string ExistingFile(const std::string &filename) {
     struct stat buffer;
     bool exist = stat(filename.c_str(), &buffer) == 0;
     bool is_dir = (buffer.st_mode & S_IFDIR) != 0;
     if(!exist) {
-        std::cerr << "File does not exist: " << filename << std::endl;
-        return false;
+        return "File does not exist: " + filename;
     } else if(is_dir) {
-        std::cerr << "File is actually a directory: " << filename << std::endl;
-        return false;
-    } else {
-        return true;
+        return "File is actually a directory: " + filename;
     }
+    return std::string();
 }
 
 /// Check for an existing directory
-inline bool ExistingDirectory(const std::string &filename) {
+inline std::string ExistingDirectory(const std::string &filename) {
     struct stat buffer;
     bool exist = stat(filename.c_str(), &buffer) == 0;
     bool is_dir = (buffer.st_mode & S_IFDIR) != 0;
     if(!exist) {
-        std::cerr << "Directory does not exist: " << filename << std::endl;
-        return false;
-    } else if(is_dir) {
-        return true;
-    } else {
-        std::cerr << "Directory is actually a file: " << filename << std::endl;
-        return false;
+        return "Directory does not exist: " + filename;
+    } else if(!is_dir) {
+        return "Directory is actually a file: " + filename;
     }
+    return std::string();
 }
 
 /// Check for a non-existing path
-inline bool NonexistentPath(const std::string &filename) {
+inline std::string NonexistentPath(const std::string &filename) {
     struct stat buffer;
     bool exist = stat(filename.c_str(), &buffer) == 0;
-    if(!exist) {
-        return true;
-    } else {
-        std::cerr << "Path exists: " << filename << std::endl;
-        return false;
+    if(exist) {
+        return "Path already exists: " + filename;
     }
+    return std::string();
 }
 
 /// Produce a range validator function
-template <typename T> std::function<bool(const std::string &)> Range(T min, T max) {
+template <typename T> std::function<std::string(const std::string &)> Range(T min, T max) {
     return [min, max](std::string input) {
         T val;
         detail::lexical_cast(input, val);
-        return val >= min && val <= max;
+        if(val < min || val > max)
+            return "Value " + input + " not in range " + std::to_string(min) + " to " + std::to_string(max);
+
+        return std::string();
     };
 }
 
 /// Range of one value is 0 to value
-template <typename T> std::function<bool(const std::string &)> Range(T max) { return Range(static_cast<T>(0), max); }
+template <typename T> std::function<std::string(const std::string &)> Range(T max) {
+    return Range(static_cast<T>(0), max);
+}
 
 /// @}
 
diff --git a/tests/AppTest.cpp b/tests/AppTest.cpp
index 00b1178b1..6c8649ab0 100644
--- a/tests/AppTest.cpp
+++ b/tests/AppTest.cpp
@@ -606,7 +606,7 @@ TEST_F(TApp, RemoveOption) {
 
 TEST_F(TApp, FileNotExists) {
     std::string myfile{"TestNonFileNotUsed.txt"};
-    EXPECT_TRUE(CLI::NonexistentPath(myfile));
+    EXPECT_NO_THROW(CLI::NonexistentPath(myfile));
 
     std::string filename;
     app.add_option("--file", filename)->check(CLI::NonexistentPath);
@@ -622,12 +622,12 @@ TEST_F(TApp, FileNotExists) {
     EXPECT_THROW(run(), CLI::ValidationError);
 
     std::remove(myfile.c_str());
-    EXPECT_FALSE(CLI::ExistingFile(myfile));
+    EXPECT_FALSE(CLI::ExistingFile(myfile).empty());
 }
 
 TEST_F(TApp, FileExists) {
     std::string myfile{"TestNonFileNotUsed.txt"};
-    EXPECT_FALSE(CLI::ExistingFile(myfile));
+    EXPECT_FALSE(CLI::ExistingFile(myfile).empty());
 
     std::string filename = "Failed";
     app.add_option("--file", filename)->check(CLI::ExistingFile);
@@ -643,7 +643,7 @@ TEST_F(TApp, FileExists) {
     EXPECT_EQ(myfile, filename);
 
     std::remove(myfile.c_str());
-    EXPECT_FALSE(CLI::ExistingFile(myfile));
+    EXPECT_FALSE(CLI::ExistingFile(myfile).empty());
 }
 
 TEST_F(TApp, InSet) {
diff --git a/tests/HelpersTest.cpp b/tests/HelpersTest.cpp
index a94583acc..e64a12e03 100644
--- a/tests/HelpersTest.cpp
+++ b/tests/HelpersTest.cpp
@@ -87,50 +87,50 @@ TEST(Trim, TrimCopy) {
 
 TEST(Validators, FileExists) {
     std::string myfile{"TestFileNotUsed.txt"};
-    EXPECT_FALSE(CLI::ExistingFile(myfile));
+    EXPECT_FALSE(CLI::ExistingFile(myfile).empty());
     bool ok = static_cast<bool>(std::ofstream(myfile.c_str()).put('a')); // create file
     EXPECT_TRUE(ok);
-    EXPECT_TRUE(CLI::ExistingFile(myfile));
+    EXPECT_TRUE(CLI::ExistingFile(myfile).empty());
 
     std::remove(myfile.c_str());
-    EXPECT_FALSE(CLI::ExistingFile(myfile));
+    EXPECT_FALSE(CLI::ExistingFile(myfile).empty());
 }
 
 TEST(Validators, FileNotExists) {
     std::string myfile{"TestFileNotUsed.txt"};
-    EXPECT_TRUE(CLI::NonexistentPath(myfile));
+    EXPECT_TRUE(CLI::NonexistentPath(myfile).empty());
     bool ok = static_cast<bool>(std::ofstream(myfile.c_str()).put('a')); // create file
     EXPECT_TRUE(ok);
-    EXPECT_FALSE(CLI::NonexistentPath(myfile));
+    EXPECT_FALSE(CLI::NonexistentPath(myfile).empty());
 
     std::remove(myfile.c_str());
-    EXPECT_TRUE(CLI::NonexistentPath(myfile));
+    EXPECT_TRUE(CLI::NonexistentPath(myfile).empty());
 }
 
 TEST(Validators, FileIsDir) {
     std::string mydir{"../tests"};
-    EXPECT_FALSE(CLI::ExistingFile(mydir));
+    EXPECT_NE(CLI::ExistingFile(mydir), "");
 }
 
 TEST(Validators, DirectoryExists) {
     std::string mydir{"../tests"};
-    EXPECT_TRUE(CLI::ExistingDirectory(mydir));
+    EXPECT_EQ(CLI::ExistingDirectory(mydir), "");
 }
 
 TEST(Validators, DirectoryNotExists) {
     std::string mydir{"nondirectory"};
-    EXPECT_FALSE(CLI::ExistingDirectory(mydir));
+    EXPECT_NE(CLI::ExistingDirectory(mydir), "");
 }
 
 TEST(Validators, DirectoryIsFile) {
     std::string myfile{"TestFileNotUsed.txt"};
-    EXPECT_TRUE(CLI::NonexistentPath(myfile));
+    EXPECT_TRUE(CLI::NonexistentPath(myfile).empty());
     bool ok = static_cast<bool>(std::ofstream(myfile.c_str()).put('a')); // create file
     EXPECT_TRUE(ok);
-    EXPECT_FALSE(CLI::ExistingDirectory(myfile));
+    EXPECT_FALSE(CLI::ExistingDirectory(myfile).empty());
 
     std::remove(myfile.c_str());
-    EXPECT_TRUE(CLI::NonexistentPath(myfile));
+    EXPECT_TRUE(CLI::NonexistentPath(myfile).empty());
 }
 
 // Yes, this is testing an app_helper :)
@@ -139,14 +139,14 @@ TEST(AppHelper, TempfileCreated) {
     {
         TempFile myfile{name};
 
-        EXPECT_FALSE(CLI::ExistingFile(myfile));
+        EXPECT_FALSE(CLI::ExistingFile(myfile).empty());
 
         bool ok = static_cast<bool>(std::ofstream(myfile.c_str()).put('a')); // create file
         EXPECT_TRUE(ok);
-        EXPECT_TRUE(CLI::ExistingFile(name));
+        EXPECT_TRUE(CLI::ExistingFile(name).empty());
         EXPECT_THROW({ TempFile otherfile(name); }, std::runtime_error);
     }
-    EXPECT_FALSE(CLI::ExistingFile(name));
+    EXPECT_FALSE(CLI::ExistingFile(name).empty());
 }
 
 TEST(AppHelper, TempfileNotCreated) {
@@ -154,9 +154,9 @@ TEST(AppHelper, TempfileNotCreated) {
     {
         TempFile myfile{name};
 
-        EXPECT_FALSE(CLI::ExistingFile(myfile));
+        EXPECT_FALSE(CLI::ExistingFile(myfile).empty());
     }
-    EXPECT_FALSE(CLI::ExistingFile(name));
+    EXPECT_FALSE(CLI::ExistingFile(name).empty());
 }
 
 TEST(AppHelper, Ofstream) {
@@ -170,9 +170,9 @@ TEST(AppHelper, Ofstream) {
             out << "this is output" << std::endl;
         }
 
-        EXPECT_TRUE(CLI::ExistingFile(myfile));
+        EXPECT_TRUE(CLI::ExistingFile(myfile).empty());
     }
-    EXPECT_FALSE(CLI::ExistingFile(name));
+    EXPECT_FALSE(CLI::ExistingFile(name).empty());
 }
 
 TEST(Split, StringList) {
diff --git a/tests/app_helper.hpp b/tests/app_helper.hpp
index 44d6b6a30..49ef7554a 100644
--- a/tests/app_helper.hpp
+++ b/tests/app_helper.hpp
@@ -27,7 +27,7 @@ class TempFile {
 
   public:
     TempFile(std::string name) : _name(name) {
-        if(!CLI::NonexistentPath(_name))
+        if(!CLI::NonexistentPath(_name).empty())
             throw std::runtime_error(_name);
     }