docopt / docopt.cpp

C++11 port of docopt
Boost Software License 1.0
1.04k stars 146 forks source link

Implement #115 #126

Closed indianakernick closed 4 years ago

indianakernick commented 4 years ago

Implements issue #115. I moved the Kind enum out into the docopt namespaces because I thought that docopt::value::Kind::StringList was an unnecessarily long name. I found out that this library uses tabs instead of spaces. The raging war between tabs and spaces continues!


I've been thinking about ways to refactor docopt::value for a while now and a few of them break backwards compatibility. Something that doesn't change the interface but cleans up the internals a little bit is the use of a visitor (much like std::visit). It could look something like the following.

struct value {
private:
  template <typename ValueA, typename ValueB, typename Func>
  static void visit(ValueA &a, ValueB &b, Func func) {
    assert(a.mKind == b.mKind);
    switch (a.mKind) {
      case Kind::Bool:
        return func(a.mVariant.boolValue, b.mVariant.boolValue);
      case Kind::Long:
        return func(a.mVariant.longValue, b.mVariant.longValue);
      case Kind::String:
        return func(a.mVariant.strValue, b.mVariant.strValue);
      case Kind::StringList:
        return func(a.mVariant.strList, b.mVariant.strList);
      case Kind::Empty: ;
    }
  }
};

That simplifies a lot of the other functions. Like operator== for example.

bool docopt::value::operator==(const value &other) const {
  if (mKind != other.mKind) return false;
  bool equal = true;
  visit(*this, other, [&equal](auto &self, auto &other) {
    equal = (self == other);
  });
  return equal;
}

Some other changes would be to rename docopt::value to docopt::Value, mark as many functions as possible with noexcept, use .hpp (C++ header file) instead of .h (C header file), also that convertToLong thing I've been rattling on about (which can be implemented in C++11 but better with C++17). Would some of these changes be accepted in a PR? What about the breaking changes? Should I stick to C++11 (it's almost 10 years old at this point)?

jaredgrubb commented 4 years ago

I'm not a big fan of "major style reformat" just for the sake of it, especially if there's something functional mixed in.

For the functional part: exposing the enum is fine. (However I would not be in favor of reimplementing operator== in terms of a visitor. We can chat about that in more detail if you want to propose it.)

For the style part: I'm not opposed to these kinds of changes, but it does a lot of churn. If it helps developers feel happier about the codebase then that would be a value. (FWIW, Yes, I hate tabs; I actually prefer using trailing-underscore these days for member variables; but what was done for reasons I cannot remember 5 years later :) ).

jaredgrubb commented 4 years ago

Can you split out the functional change from the style changes (as separate PR)?

indianakernick commented 4 years ago

@jaredgrubb By style changes, do you mean mKind and mVariant? I had to rename kind to mKind to expose the kind function. I renamed variant just to be consistent. Would you prefer kind_ and variant_ instead?

jaredgrubb commented 4 years ago

I see... I'd prefer to rename them to "kind" and "variant" in that case. If we're going to change the style, that's my preference :)

indianakernick commented 4 years ago

@jaredgrubb Done

indianakernick commented 4 years ago

Is there a reason this hasn't been merged? Would you like me to make some changes?

indianakernick commented 4 years ago

Beginning to feel like I'm talking to a brick wall here... 😅

jaredgrubb commented 4 years ago

There is no good reason it hasn't been merged :)

jaredgrubb commented 4 years ago

Thanks for your patch!