Allow general flag types

[phlptp]

If merged this pull request will allow general flag values to be used, along with customized default values. It also introduces a shortcut syntax for retrieving flag and option values. Which is useful for allowing options with no storage variables.

app["--flag"].as<std::string>() 

this is similar notation to some other Command line libraries and could make transitioning to CLI easier

It also allows numbers to be used as short flags, so -7 and similar is now allowed

This could be specified to have a default value like -7{7} in which case if the value -7 were passed it would produce a result of 7 in the output.

This PR addresses #123 and #124.

[codecov]

Codecov Report

Merging #235 into master will not change coverage.
The diff coverage is 100%.

@@          Coverage Diff           @@
##           master   #235    +/-   ##
======================================
  Coverage     100%   100%            
======================================
  Files          12     12            
  Lines        2112   2212   +100     
======================================
+ Hits         2112   2212   +100

Impacted Files	Coverage Δ
include/CLI/Option.hpp	100% <100%> (ø)	⬆️
include/CLI/App.hpp	100% <100%> (ø)	⬆️
include/CLI/StringTools.hpp	100% <100%> (ø)	⬆️
include/CLI/TypeTools.hpp	100% <100%> (ø)	⬆️
include/CLI/Split.hpp	100% <100%> (ø)	⬆️
include/CLI/ConfigFwd.hpp	100% <100%> (ø)	⬆️
include/CLI/Error.hpp	100% <100%> (ø)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1a1cde9...700db24. Read the comment docs.

[codecov]

Codecov Report

Merging #235 into master will decrease coverage by 0.9%.
The diff coverage is 90.75%.

@@            Coverage Diff             @@
##           master     #235      +/-   ##
==========================================
- Coverage     100%   99.09%   -0.91%     
==========================================
  Files          12       12              
  Lines        2087     2102      +15     
==========================================
- Hits         2087     2083       -4     
- Misses          0       19      +19

Impacted Files	Coverage Δ
include/CLI/App.hpp	99.76% <100%> (-0.24%)	⬇️
include/CLI/StringTools.hpp	100% <100%> (ø)	⬆️
include/CLI/TypeTools.hpp	100% <100%> (ø)	⬆️
include/CLI/Split.hpp	100% <100%> (ø)	⬆️
include/CLI/ConfigFwd.hpp	100% <100%> (ø)	⬆️
include/CLI/Option.hpp	94.84% <65.21%> (-5.16%)	⬇️
include/CLI/Validators.hpp	100% <0%> (ø)	⬆️
... and 2 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 17d05b0...abc972c. Read the comment docs.

[phlptp]

One question is whether there should be a method on options to disable flag value overrides. for example you are able to specify
add_flag("--count{345}");

but on the command line --count=45
The question is should there be a way to disallow the =45 as an override? and if that were disabled what should the behavior be (ignore, or error)?

[henryiii]

Not sure about the proper way to disable it, but I think an error is better than ignoring an issue.

PS: I'll be traveling on vacation from tomorrow till Tuesday, so might not be able to do very complicated reviews for a while.

[phlptp]

TODOs

• update documentation/ReadME
• some code cleanup with in code documentation and better consistency with rest of code

[phlptp]

For @henryiii
regarding the operator[], do you think it should return a const Option & or a const Option *?

And I made the operator[] function a template so there is only one in the code, but it may have the potential to generate a bunch of symbols if different string constants are used, so I may prefer the two overloads without the template, but I think either might be valid and have advantages/disadvantages, so i think it is your call?

[henryiii]

I generally avoid returning references, because this is not safe:

int& value(int& x) {
    return x;
}

int main() {
    int x = 3;
    auto y = value(x);
    y = 4;
    
    std::cout << x << " and " << y << std::endl;
}

You have to write auto& to get the correct behavior.

Now, disallowing copies helps somewhat, but it still might be more surprising to a novice programmer. Pointers are a bit more common and it's harder to make a mistake with auto.

[henryiii]

The question is should there be a way to disallow the =45 as an override?

Just a thought; should flags that have defaults automatically get this disabled? That is,

->add_flag("--flag,--no-flag{false}", ...);

Maybe should allow --flag=false but probably not --no-flag=true, which doesn't even do what you might read for it anyway.

Edit: note this is two suggestions, the one below is an extension of this idea (possibly not a good one).

---

In fact, it seems like that almost negates the need for the explicit setting; if a user did this:

->add_flag("--flag{true},--no-flag{false}", ...);

That would mean no = syntax allowed. That would also indicate that all simple classic style options would support the = syntax by default, and the verbose syntax would be required instead of an inheritable setting like you have now, I'm not sure what I think of that.

[henryiii]

FYI, in the "files changed" tab you can queue up a list of accepted suggestions and commit them in one go. ;)

[phlptp]

I will change [] to return by pointer.

The question is should there be a way to disallow the =45 as an override?

Just a thought; should flags that have defaults automatically get this disabled? That is,

->add_flag("--flag,--no-flag{false}", ...);

Maybe should allow --flag=false but probably not --no-flag=true, which doesn't even do what you might read for it anyway.

In fact, it seems like that almost negates the need for the explicit setting; if a user did this:

->add_flag("--flag{true},--no-flag{false}", ...);

That would mean no = syntax allowed. That would also indicate that all simple classic style options would support the = syntax by default, and the verbose syntax would be required instead of an inheritable setting like you have now, I'm not sure what I think of that.

My hesitancy with that would be

->add_flag("--flag,--no-flag{false}", ...);

you might still want to allow --flag=false, so I would a little hesitant to enable that by default with mixed usage, though it might be advisable if all flags list defaults.

Also even if the disable_flag_override is selected if you specify ={} or =false then there is no error.

right now if you were to do --no-flag=false it would be the equivalent of --flag=true you get the double negative thing going on.

[henryiii]

I haven't checked the code in detail, so not sure if it's even possible, but in the first half of that I was suggesting that

--flag,--no-flag{false}

Would produce a half-way state, where --flag=false would work, and --no-flag=true would fail. Setting the disable_flag_override can't affect an individual flag name, so that would put you into an all or nothing state.

[henryiii]

We can merge, play with it a while, then tweak if needed.

[phlptp]

FYI, in the "files changed" tab you can queue up a list of accepted suggestions and commit them in one go. ;)

Did not know that, Our code reviews haven't used suggestions that much, maybe should start using that feature a little more

[phlptp]

do you have an opinion on the operator[] template, whether it should remain a template or split into 2 non-template overloads?

[henryiii]

Since it's a simple function, I'd prefer avoiding the template (mildly). Simpler compile, doesn't create extra symbols for different string types. Slightly nicer error message too. Not a strong preference though. And on the other side, you only have to have one unit test for the template instead of two. If was any longer, I'd lean toward the template instead (or having one call the other; rather makes sense since you are working around a compiler "feature" for operator[].

[henryiii]

Did not know that

It's pretty new, it might still even list "experimental" when using it. It's quite handy though.

[phlptp]

I moved to two functions and collapsed all the commits.

I think I would lean away from ever putting on disable_flag_override by default. One of the drivers was a use case to have a default output for flags for example

std::string file;
app.add_flag("--output{out.txt}",file,"enable file out and specify file");

In this case it is expected overrides would be fairly common and you wouldn't want to disable the overrides.

[phlptp]

I think this is ready.