Skip to content

Implement #115 - #126

Merged
jaredgrubb merged 4 commits into
docopt:masterfrom
indianakernick:issue_115
Apr 15, 2020
Merged

jaredgrubb merged 4 commits into
docopt:masterfrom
indianakernick:issue_115

Conversation

@indianakernick

@indianakernick indianakernick commented Dec 13, 2019 •

Copy link
Copy Markdown
Contributor

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

Copy link
Copy Markdown
Member

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

Copy link
Copy Markdown
Member

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

@indianakernick

indianakernick commented Feb 14, 2020 •

Copy link
Copy Markdown
Contributor Author

@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

Copy link
Copy Markdown
Member

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

Copy link
Copy Markdown
Contributor Author

@jaredgrubb Done

@indianakernick

Copy link
Copy Markdown
Contributor Author

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

@indianakernick

Copy link
Copy Markdown
Contributor Author

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

@jaredgrubb

Copy link
Copy Markdown
Member

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

@jaredgrubb
jaredgrubb merged commit c8a63d5 into docopt:master Apr 15, 2020
@jaredgrubb

Copy link
Copy Markdown
Member

Thanks for your patch!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants