docopt / docopt.cpp

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

Help offered #120

Closed tdegeus closed 1 year ago

tdegeus commented 4 years ago

It seems that this repository is not very active, which is a shame as there seem to be some interesting PRs waiting to be reviewed/merged. It could be great to give one or more contributors push rights to help maintaining this library.

tdegeus commented 4 years ago

P.S. I happily offer my services

indianakernick commented 4 years ago

The code could certainly be modernised. For example, docopt::value could be replaced with (or implemented using) std::variant. std::unordered_map could probably be used instead of std::map. There are probably a few places where a std::string_view could be used instead of std::string. Likewise for std::span and std::vector. An anonymous namespace could be used instead of static. The library can be used as a header-only library via DOCOPT_INLINE so it would be nice if there was a script that generated a single header like what is done for other header-only libraries. If we're using modern compilers then I hardly see the point in dealing with boost::regex.

On the other hand, if it works, it works. Most of what I just said is modern for the sake of being modern. This isn't a template heavy library where modern compilers means more features for the user. This also isn't a performance critical library where using std::unordered_map instead of std::map would actually make any noticeable difference.

Now I'm not really sure what the point of this comment is.

tdegeus commented 4 years ago

@Kerndog73 Thanks for your comments! I should (clearly) clarify my comment. Thereto I start by saying: Thanks for the library! It simplifies my life, and I stand grateful for it.

Although some of the things you describe would indeed be a modernisation, I too believe that one should not modernise just for the sake of modernising. I believe that in particular for the functionality that docopt provides one should not worry too much about efficiency as the docopt-bit of a code is highly unlikely to be time critical for a C++ code. That just leaves user-API as the only point of modernisation. From your comments that would be primarily heaving a single-file header-only library and the suppression of boost::regex.

But my comment not even came from those two points. Rather, merely from an observation of activity. For example, PRs #105 and #112 seem to offer an enhanced user interaction. But, from a big distance, they do not seem to be considered (neither accepted, not rejected). Another point is that the CI seems to be outdated. Again, from a big distance, that could make one worry that the library is not actively maintained, possibly make a (potential) user worry to find oneself in a fragile position should a bug (or urgent question) come up.

indianakernick commented 4 years ago

@tdegeus

Thanks for the library!

Don't thank me! 😄 I'm just a guy who happened to see this issue. I'm not a contributor.

As for modernising the interface, DocoptArgumentError could probably be renamed to ArgumentError because docopt::DocoptArgumentError looks weird. Anyway, I see your point. I might be willing to contribute if I knew someone was actually going to look at my pull requests!

Another thing I just thought of: a type alias for std::map<std::string, docopt::value> would be nice. Maybe docopt::options or something.

jaredgrubb commented 4 years ago

I'm definitely here and have fixed any bugs that are reported.

Regarding the use of boost::regex, that is optional. By default <regex> is used, but some of the regex implementations are very poor (eg, the MSVC regex implementation overflows the stack on family simple uses of this library). So the boost::regex is provided as optional for those that prefer to use it (boost::regex is a very good implementation).

Regarding using std::variant and string_view and the like, I like the idea. However that would raise the C++ version requirement to C++17 or later. I am open to the idea, but it would be a break requirement. Not sure if that should be done on a branch -- or even a fork. Of course we should see that there is benefit from it in some form, as change for change's sake can do more harm.

indianakernick commented 4 years ago

change for change's sake can do more harm

All three of us agree on that.

I think there's a lot of minor (but breaking) improvements that could be made to the API even when just sticking to C++11 (mostly just renaming things and maybe things like #115). Though if you're making breaking changes anyway then you might as well take advantage of C++17 if it helps make a better API. Something like this should belong in a branch.

Anyway, I'm getting away from the main point of this issue. I'd be happy to help out and @tdegeus is happy to help out too.

jaredgrubb commented 4 years ago

I'm happy to have help! Let me know what you need and I can help set things up.

tdegeus commented 4 years ago

Sound good!

I guess it is mostly important to know that you are there. I'm happy to co-review or submit PRs if I know that things are integrated in finite time.

tdegeus commented 4 years ago

Though, if not the PRs' authors, somebody with push-rights has to resolve conflicts and retrigger the CI (as soon as #121 is integrated) on some of the older PRs