- Notifications
You must be signed in to change notification settings - Fork146
Added ValueOr functions to ease usage of docopt in -fno-exceptions environment#137
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:master
Are you sure you want to change the base?
Uh oh!
There was an error while loading.Please reload this page.
Conversation
thelema left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
These changes look like strict improvements. Technically there may be a slight increase in compile time, but the new APIs look convenient. It might be good to add some tests exercising the new APIs, but I would accept these changes just based on code review, and allow tests to be added later.
| } | ||
| return variant_.longValue; | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Could we maybe put this in a private function to be shared with both? That maybe returns std::optional, and one would throw and the other would not?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Sorry for my late answer. As far as I knowstd::optional was added to the STL withC++17 so using it here would mean to either raise the requiredC++ version fromC++11 toC++17 or having multiple implementations depending on the C++ version. I think neither is desirable.
What I could see working is either useboost::optional asboost is already a dependency because ofboost::regex or usingSFINAE to abstract the optional container away like I did in this "simple" example I wrote herehttps://godbolt.org/z/YPejv9Y71
(gist mirror:https://gist.github.com/Dadie/d06ea6aca53d37142577b4d1ce305246 )
The later would also enable the usage of other container classes aside fromboost::optional likestd::optional but probably introduce unnecessary complexity and might lead to template code explosion. While the former might break some projects depending on docopt as they may not have boost::optional available on their platform.
While I'm not the maintainer of this library (so it's luckily not me who should decide on it) I'm sort of reluctant in neither solution but am open for opinions.
Half-OT:
On another note I'm quite taken by the idea of porting this library toC++20. I think this type of library would greatly benefit from newer language features likeconsteval and newer STL classes likestd::string_view and/orstd::variant. But (sadly a big but) adoption rate of modern C++ is slow and I would guess it will take at least another 5-8 years till we can assumeC++20 being the baseline. So switching now to a newer standard would do nothing aside from breaking a lot of projects and making the library literally unusable for a lot of people.
Add docopt::value::asStringOr(string) functionAdd docopt::value::asLongOr(long) functionAdd docopt::value::asStringList(StringList) function
Dadie commentedJun 21, 2022 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
I'd love to add some tests for the functions but I'm not sure where I should add them as the current test I'd assume the following cases would be helpful: const std::string test_str ="STR";const std::string test_other_str ="OTHER STR";const std::string test_num_str ="123";const std::vector<std::string> test_str_list = {test_str};const std::vector<std::string> test_other_str_list = {test_other_str};// Case 0: Bool Value (true)constauto v_true = docopt::value(true);assert(v_true.asBoolOr(true) ==true);// <-- This should not use the Or-Valueassert(v_true.asBoolOr(false) ==true);// <-- This should not use the Or-Valueassert(v_true.asLongOr(-1) == -1);assert(v_true.asStringOr(test_str) == test_str);assert(v_true.asStringListOr(test_str_list) == test_str_list);// Case 1: Bool Value (false)constauto v_false = docopt::value(false);assert(v_false.asBoolOr(true) ==false);// <-- This should not use the Or-Valueassert(v_false.asBoolOr(false) ==false);// <-- This should not use the Or-Valueassert(v_false.asLongOr(-1) == -1);assert(v_false.asLongOr(54321) ==54321);assert(v_false.asStringOr(test_str) == test_str);assert(v_false.asStringListOr(test_str_list) == test_str_list);// Case 2: Long Valueconstauto v_long = docopt::value(12345);assert(v_long.asBoolOr(true) ==true);assert(v_long.asBoolOr(false) ==false);assert(v_long.asLongOr(-1) ==12345);// <-- This should not use the Or-Valueassert(v_long.asLongOr(54321) ==12345);// <-- This should not use the Or-Valueassert(v_long.asStringOr(test_str) == test_str);assert(v_long.asStringListOr(test_str_list) == test_str_list);// Case 3: String (Non-Numeric)constauto v_str = docopt::value(test_other_str);assert(v_str.asBoolOr(true) ==true);assert(v_str.asBoolOr(false) ==false);assert(v_str.asLongOr(-1) == -1);assert(v_str.asLongOr(54321) ==54321);assert(v_str.asStringOr(test_str) == test_other_str);// <-- This should not use the Or-Valueassert(v_str.asStringListOr(test_str_list) == test_str_list);// Case 4: String (Numeric)constauto v_num_str = docopt::value(test_num_str);assert(v_num_str.asBoolOr(true) ==true);assert(v_num_str.asBoolOr(false) ==false);assert(v_num_str.asLongOr(-1) ==123);// <-- This should not use the Or-Valueassert(v_num_str.asLongOr(54321) ==123);// <-- This should not use the Or-Valueassert(v_num_str.asStringOr(test_str) == test_num_str);// <-- This should not use the Or-Valueassert(v_num_str.asStringListOr(test_str_list) == test_str_list);// Case 5: String Listconstauto v_str_list = docopt::value(test_other_str_list);assert(v_str_list.asBoolOr(true) ==true);assert(v_str_list.asBoolOr(false) ==false);assert(v_str_list.asLongOr(-1) == -1);assert(v_str_list.asLongOr(54321) ==54321);assert(v_str_list.asStringOr(test_str) == test_other_str);assert(v_str_list.asStringListOr(test_str_list) == test_other_str_list);// <-- This should not use the Or-Value |
While parsing of the documentation with
docopt::docopt()is-fno-exceptionsenvironment friendly, accessing any parsed value is not and requires some boilerplate code likestd::map< std::string, docopt::value > args;{ args =docopt::docopt(USAGE, { argv +1, argv + argc },true,// show help if requested"my_programm 1.0.0");// version string}/* ...*/auto v = args.contains("123") ? ( args.at("123").isString() ? args.at("123").asString() :"Fallback Value" ) :"Fallback Value";which can at least be simplified like this with the PR
std::map< std::string, docopt::value > args;{ args =docopt::docopt(USAGE, { argv +1, argv + argc },true,// show help if requested"my_programm 1.0.0");// version string}/* ...*/auto v = args.contains("123") ? args.at("123").asStringOr("Fallback Value") :"Fallback Value";or for those who don't care about unnecessary allocations like this
std::map< std::string, docopt::value > args;{ args =docopt::docopt(USAGE, { argv +1, argv + argc },true,// show help if requested"my_programm 1.0.0");// version string}/* ...*/auto v = args["123"].asStringOr("Fallback Value");