mesos-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From b...@apache.org
Subject [2/2] mesos git commit: Refactored common functionality into FlagsBase.
Date Mon, 01 Jun 2015 23:00:13 GMT
Refactored common functionality into FlagsBase.

Jira: MESOS-2711

Every program that uses stout's `FlagsBase` ends up re-implementing
the `usage()` function, and adding a `bool help` (and associated
--help flag); this functionality has now been refactored in the base
class and is available everywhere.

This change attempts to be backward-compatible, so it does not alter
the behavior of the program when --help is invoked (by, e.g.,
automatically printing usage and exiting) but leaves up to the caller
to check for `flags.help` and then decide what action to take.

There is now a default behavior for the "leader" ("Usage: <program
name> [options]") but the client API allows to modify that too.

Note - anywhere I found the use of the `--help` flag the behavior was
the same: print usage and exit (see also
https://reviews.apache.org/r/34195).

Review: https://reviews.apache.org/r/34193


Project: http://git-wip-us.apache.org/repos/asf/mesos/repo
Commit: http://git-wip-us.apache.org/repos/asf/mesos/commit/1694bda8
Tree: http://git-wip-us.apache.org/repos/asf/mesos/tree/1694bda8
Diff: http://git-wip-us.apache.org/repos/asf/mesos/diff/1694bda8

Branch: refs/heads/master
Commit: 1694bda8d8b7a43ac8fd857c5909d8a1a25f2754
Parents: f855fb7
Author: Marco Massenzio <marco@mesosphere.io>
Authored: Mon Jun 1 03:49:00 2015 -0700
Committer: Benjamin Hindman <benjamin.hindman@gmail.com>
Committed: Mon Jun 1 15:59:38 2015 -0700

----------------------------------------------------------------------
 .../stout/include/stout/flags/flags.hpp         | 126 +++++++++++++++----
 .../3rdparty/stout/tests/flags_tests.cpp        |  70 ++++++++++-
 2 files changed, 171 insertions(+), 25 deletions(-)
----------------------------------------------------------------------


http://git-wip-us.apache.org/repos/asf/mesos/blob/1694bda8/3rdparty/libprocess/3rdparty/stout/include/stout/flags/flags.hpp
----------------------------------------------------------------------
diff --git a/3rdparty/libprocess/3rdparty/stout/include/stout/flags/flags.hpp b/3rdparty/libprocess/3rdparty/stout/include/stout/flags/flags.hpp
index fb383b4..61a405f 100644
--- a/3rdparty/libprocess/3rdparty/stout/include/stout/flags/flags.hpp
+++ b/3rdparty/libprocess/3rdparty/stout/include/stout/flags/flags.hpp
@@ -42,6 +42,7 @@ namespace flags {
 class FlagsBase
 {
 public:
+  FlagsBase() { add(&help, "help", "Prints this help message", false); }
   virtual ~FlagsBase() {}
 
   // Load any flags from the environment given the variable prefix,
@@ -64,11 +65,11 @@ public:
   // Load any flags from the environment as above but remove processed
   // flags from 'argv' and update 'argc' appropriately. For example:
   //
-  // argv = ["/path/to/program", "--arg1", "hello", "--arg2", "--", "world"]
+  // argv = ["/path/program", "--arg1", "hi", "--arg2", "--", "bye"]
   //
   // Becomes:
   //
-  // argv = ["/path/to/program", "hello", "world"]
+  // argv = ["/path/program", "hi", "bye"]
   virtual Try<Nothing> load(
       const Option<std::string>& prefix,
       int* argc,
@@ -84,18 +85,57 @@ public:
       const std::map<std::string, std::string>& values,
       bool unknowns = false);
 
-  // Returns a string describing the flags.
-  std::string usage() const;
+  // Returns a string describing the flags, preceded by a "usage
+  // message" that will be prepended to that description (see
+  // 'FlagsBase::usageMessage_').
+  //
+  // The optional 'message' passed to this function will be prepended
+  // to the generated string returned from this function.
+  //
+  // Derived classes and clients can modify the standard usage message
+  // by setting it before calling this method via 'setUsageMessage()'.
+  //
+  // This allows one to set a generic message that will be used for
+  // each invocation of this method, and to additionally add one for
+  // the particular invocation:
+  //
+  //    MyFlags flags;
+  //    flags.setUsageMessage("Custom Usage Message");
+  //    ...
+  //    if (flags.foo.isNone()) {
+  //      std::cerr << flags.usage("Missing required --foo flag");
+  //    }
+  //
+  // The 'message' would be emitted first, followed by a blank line,
+  // then the 'usageMessage_', finally followed by the flags'
+  // description, for example:
+  //
+  //    Missing required --foo flag
+  //
+  //    Custom Usage Message
+  //
+  //      --[no-]help       Prints this help message. (default: false)
+  //      --foo=VALUE       Description about 'foo' here.
+  //      --bar=VALUE       Description about 'bar' here. (default: 42)
+  //
+  std::string usage(const Option<std::string>& message = None()) const;
+
+  // Sets the default message that is prepended to the flags'
+  // description in 'usage()'.
+  void setUsageMessage(const std::string& message)
+  {
+    usageMessage_ = Some(message);
+  }
 
   typedef std::map<std::string, Flag>::const_iterator const_iterator;
 
-  const_iterator begin() const { return flags.begin(); }
-  const_iterator end() const { return flags.end(); }
+  const_iterator begin() const { return flags_.begin(); }
+  const_iterator end() const { return flags_.end(); }
 
   typedef std::map<std::string, Flag>::iterator iterator;
 
-  iterator begin() { return flags.begin(); }
-  iterator end() { return flags.end(); }
+  iterator begin() { return flags_.begin(); }
+  iterator end() { return flags_.end(); }
 
   template <typename T1, typename T2>
   void add(T1* t1,
@@ -122,12 +162,31 @@ protected:
 
   void add(const Flag& flag);
 
+public:
+  // TODO(marco): IMO the entire --help functionality should be
+  // encapsulated inside the FlagsBase class.
+  // For now, exposing this for the caller(s) to decide what to
+  // do when the user asks for help.
+  bool help;
+
+protected:
+  // The program's name, extracted from argv[0] by default;
+  // declared 'protected' so that derived classes can alter this
+  // behavior.
+  std::string programName_;
+
+  // An optional custom usage message, will be printed 'as is'
+  // just above the list of flags and their explanation.
+  // It is 'None' by default, in which case the default
+  // behavior is to print "Usage:" followed by the 'programName_'.
+  Option<std::string> usageMessage_;
+
 private:
   // Extract environment variable "flags" with the specified prefix.
   std::map<std::string, Option<std::string> > extract(
       const std::string& prefix);
 
-  std::map<std::string, Flag> flags;
+  std::map<std::string, Flag> flags_;
 };
 
 
@@ -141,7 +200,7 @@ class _Flags4 : public virtual FlagsBase {};
 class _Flags5 : public virtual FlagsBase {};
 
 
-// TODO(benh): Add some "type constraints" for template paramters to
+// TODO(benh): Add some "type constraints" for template parameters to
 // make sure they are all of type FlagsBase.
 template <typename Flags1 = _Flags1,
           typename Flags2 = _Flags2,
@@ -287,14 +346,14 @@ void FlagsBase::add(
 
 inline void FlagsBase::add(const Flag& flag)
 {
-  if (flags.count(flag.name) > 0) {
+  if (flags_.count(flag.name) > 0) {
     EXIT(1) << "Attempted to add duplicate flag '" << flag.name << "'";
   } else if (flag.name.find("no-") == 0) {
     EXIT(1) << "Attempted to add flag '" << flag.name
             << "' that starts with the reserved 'no-' prefix";
   }
 
-  flags[flag.name] = flag;
+  flags_[flag.name] = flag;
 }
 
 
@@ -312,8 +371,8 @@ inline std::map<std::string, Option<std::string> > FlagsBase::extract(
       name = strings::lower(name); // Allow PREFIX_NAME or PREFIX_name.
 
       // Only add if it's a known flag.
-      if (flags.count(name) > 0 ||
-          (name.find("no-") == 0 && flags.count(name.substr(3)) > 0)) {
+      if (flags_.count(name) > 0 ||
+          (name.find("no-") == 0 && flags_.count(name.substr(3)) > 0)) {
         values[name] = Some(value);
       }
     }
@@ -338,10 +397,15 @@ inline Try<Nothing> FlagsBase::load(
 {
   std::map<std::string, Option<std::string> > values;
 
+  // Grab the program name from argv[0].
+  programName_ = argc > 0 ? os::basename(argv[0]).get() : "";
+
   if (prefix.isSome()) {
     values = extract(prefix.get());
   }
 
+
+
   // Read flags from the command line.
   for (int i = 1; i < argc; i++) {
     const std::string arg(strings::trim(argv[i]));
@@ -398,6 +462,9 @@ inline Try<Nothing> FlagsBase::load(
     values = extract(prefix.get());
   }
 
+  // Grab the program name from argv, without removing it.
+  programName_ = argc > 0 ? os::basename(*(argv[0])).get() : "";
+
   // Keep the arguments that are not being processed as flags.
   std::vector<char*> args;
 
@@ -477,30 +544,30 @@ inline Try<Nothing> FlagsBase::load(
     const std::string& name = iterator->first;
     const Option<std::string>& value = iterator->second;
 
-    if (flags.count(name) > 0) {
+    if (flags_.count(name) > 0) {
       if (value.isSome()) {                        // --name=value
-        if (flags[name].boolean && value.get() == "") {
-          flags[name].loader(this, "true"); // Should never fail.
+        if (flags_[name].boolean && value.get() == "") {
+          flags_[name].loader(this, "true"); // Should never fail.
         } else {
-          Try<Nothing> loader = flags[name].loader(this, value.get());
+          Try<Nothing> loader = flags_[name].loader(this, value.get());
           if (loader.isError()) {
             return Error(
                 "Failed to load flag '" + name + "': " + loader.error());
           }
         }
       } else {                                     // --name
-        if (flags[name].boolean) {
-          flags[name].loader(this, "true"); // Should never fail.
+        if (flags_[name].boolean) {
+          flags_[name].loader(this, "true"); // Should never fail.
         } else {
           return Error(
               "Failed to load non-boolean flag '" + name + "': Missing value");
         }
       }
     } else if (name.find("no-") == 0) {
-      if (flags.count(name.substr(3)) > 0) {       // --no-name
-        if (flags[name.substr(3)].boolean) {
+      if (flags_.count(name.substr(3)) > 0) {       // --no-name
+        if (flags_[name.substr(3)].boolean) {
           if (value.isNone() || value.get() == "") {
-            flags[name.substr(3)].loader(this, "false"); // Should never fail.
+            flags_[name.substr(3)].loader(this, "false"); // Should never fail.
           } else {
             return Error(
                 "Failed to load boolean flag '" + name.substr(3) +
@@ -540,12 +607,22 @@ inline Try<Nothing> FlagsBase::load(
 }
 
 
-inline std::string FlagsBase::usage() const
+inline std::string FlagsBase::usage( const Option<std::string>& message) const
 {
   const int PAD = 5;
 
   std::string usage;
 
+  if (message.isSome()) {
+    usage = message.get() + "\n\n";
+  }
+
+  if (usageMessage_.isNone()) {
+    usage += "Usage: " + programName_ + " [options]\n\n";
+  } else {
+    usage += usageMessage_.get() + "\n\n";
+  }
+
   std::map<std::string, std::string> col1; // key -> col 1 string.
 
   // Construct string for the first column and store width of column.
@@ -581,6 +658,7 @@ inline std::string FlagsBase::usage() const
       usage += line;
     }
   }
+
   return usage;
 }
 

http://git-wip-us.apache.org/repos/asf/mesos/blob/1694bda8/3rdparty/libprocess/3rdparty/stout/tests/flags_tests.cpp
----------------------------------------------------------------------
diff --git a/3rdparty/libprocess/3rdparty/stout/tests/flags_tests.cpp b/3rdparty/libprocess/3rdparty/stout/tests/flags_tests.cpp
index 0028119..a6e8ba9 100644
--- a/3rdparty/libprocess/3rdparty/stout/tests/flags_tests.cpp
+++ b/3rdparty/libprocess/3rdparty/stout/tests/flags_tests.cpp
@@ -19,9 +19,17 @@
 
 using namespace flags;
 
+using std::cout;
+using std::endl;
 using std::string;
 using std::map;
 
+
+// Just used to test that the default implementation
+// of --help and 'usage()' works as intended.
+class EmptyFlags : public FlagsBase {};
+
+
 class TestFlags : public virtual FlagsBase
 {
 public:
@@ -478,11 +486,15 @@ TEST(FlagsTest, Errors)
 }
 
 
-TEST(FlagsTest, Usage)
+TEST(FlagsTest, UsageMessage)
 {
   TestFlags flags;
+  flags.setUsageMessage("This is a test");
 
   EXPECT_EQ(
+      "This is a test\n"
+      "\n"
+      "  --[no-]help       Prints this help message (default: false)\n"
       "  --name1=VALUE     Set name1 (default: ben folds)\n"
       "  --name2=VALUE     Set name2 (default: 42)\n"
       "  --[no-]name3      Set name3 (default: false)\n"
@@ -492,6 +504,62 @@ TEST(FlagsTest, Usage)
 }
 
 
+TEST(FlagsTest, EmptyUsage)
+{
+  EmptyFlags flags;
+
+  EXPECT_EQ(
+      "Usage:  [options]\n"
+      "\n"
+      "  --[no-]help     Prints this help message (default: false)\n",
+      flags.usage());
+}
+
+
+TEST(FlagsTest, ProgramName)
+{
+  // To test with a custom program name.
+  class MyTestFlags : public TestFlags
+  {
+  public:
+    MyTestFlags() { programName_ = "TestProgram"; }
+  };
+
+
+  MyTestFlags flags;
+
+  EXPECT_EQ(
+      "Usage: TestProgram [options]\n"
+      "\n"
+      "  --[no-]help       Prints this help message (default: false)\n"
+      "  --name1=VALUE     Set name1 (default: ben folds)\n"
+      "  --name2=VALUE     Set name2 (default: 42)\n"
+      "  --[no-]name3      Set name3 (default: false)\n"
+      "  --[no-]name4      Set name4\n"
+      "  --[no-]name5      Set name5\n",
+      flags.usage());
+}
+
+
+TEST(FlagsTest, OptionalMessage)
+{
+  TestFlags flags;
+
+  EXPECT_EQ(
+      "Good news: this test passed!\n"
+      "\n"
+      "Usage:  [options]\n"
+      "\n"
+      "  --[no-]help       Prints this help message (default: false)\n"
+      "  --name1=VALUE     Set name1 (default: ben folds)\n"
+      "  --name2=VALUE     Set name2 (default: 42)\n"
+      "  --[no-]name3      Set name3 (default: false)\n"
+      "  --[no-]name4      Set name4\n"
+      "  --[no-]name5      Set name5\n",
+      flags.usage("Good news: this test passed!"));
+}
+
+
 TEST(FlagsTest, Duration)
 {
   Flags<TestFlags> flags;


Mime
View raw message