Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Help offered #120

Closed
tdegeus opened this issue Nov 21, 2019 · 9 comments
Closed

Help offered #120

tdegeus opened this issue Nov 21, 2019 · 9 comments

Comments

@tdegeus
Copy link
Contributor

tdegeus commented Nov 21, 2019

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
Copy link
Contributor Author

tdegeus commented Nov 21, 2019

P.S. I happily offer my services

@indianakernick
Copy link
Contributor

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
Copy link
Contributor Author

tdegeus commented Nov 27, 2019

@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
Copy link
Contributor

indianakernick commented Nov 27, 2019

@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
Copy link
Member

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
Copy link
Contributor

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
Copy link
Member

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

@tdegeus
Copy link
Contributor Author

tdegeus commented Nov 29, 2019

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
Copy link
Contributor Author

tdegeus commented Nov 29, 2019

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

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

No branches or pull requests

3 participants