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

Added ValueOr functions to ease usage of docopt in -fno-exceptions environment #137

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

Dadie
Copy link

@Dadie Dadie commented Sep 22, 2020

While parsing of the documentation with docopt::docopt() is -fno-exceptions environment friendly, accessing any parsed value is not and requires some boilerplate code like

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").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");

Copy link

@thelema thelema left a 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;
}

Copy link
Member

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?

Copy link
Author

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 know std::optional was added to the STL with C++17 so using it here would mean to either raise the required C++ version from C++11 to C++17 or having multiple implementations depending on the C++ version. I think neither is desirable.

What I could see working is either use boost::optional as boost is already a dependency because of boost::regex or using SFINAE to abstract the optional container away like I did in this "simple" example I wrote here https://godbolt.org/z/YPejv9Y71
(gist mirror: https://gist.github.com/Dadie/d06ea6aca53d37142577b4d1ce305246 )

The later would also enable the usage of other container classes aside from boost::optional like std::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 to C++20. I think this type of library would greatly benefit from newer language features like consteval and newer STL classes like std::string_view and/or std::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 assume C++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) function
Add docopt::value::asLongOr(long) function
Add docopt::value::asStringList(StringList) function
@Dadie
Copy link
Author

Dadie commented Jun 21, 2022

I'd love to add some tests for the functions but I'm not sure where I should add them as the current test run_test.py using the compiled run_testcase.cpp doesn't seem to be optimal to test those functions.

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)
const auto v_true = docopt::value(true);
assert(v_true.asBoolOr(true) == true); // <-- This should not use the Or-Value
assert(v_true.asBoolOr(false) == true); // <-- This should not use the Or-Value
assert(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)
const auto v_false = docopt::value(false);
assert(v_false.asBoolOr(true) == false); // <-- This should not use the Or-Value
assert(v_false.asBoolOr(false) == false); // <-- This should not use the Or-Value
assert(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 Value
const auto 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-Value
assert(v_long.asLongOr(54321) == 12345); // <-- This should not use the Or-Value
assert(v_long.asStringOr(test_str) == test_str);
assert(v_long.asStringListOr(test_str_list) == test_str_list);

// Case 3: String (Non-Numeric)
const auto 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-Value
assert(v_str.asStringListOr(test_str_list) == test_str_list);

// Case 4: String (Numeric)
const auto 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-Value
assert(v_num_str.asLongOr(54321) == 123); // <-- This should not use the Or-Value
assert(v_num_str.asStringOr(test_str) == test_num_str); // <-- This should not use the Or-Value
assert(v_num_str.asStringListOr(test_str_list) == test_str_list);

// Case 5: String List
const auto 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

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.

3 participants