Allow general flag types #235

phlptp · 2019-02-20T16:07:25Z

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 · 2019-02-20T17:08:17Z

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 · 2019-02-20T17:08:17Z

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 · 2019-02-20T20:14:18Z

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 · 2019-02-20T21:55:51Z

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.

examples/CMakeLists.txt

include/CLI/App.hpp

phlptp · 2019-02-21T19:17:28Z

TODOs

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

phlptp · 2019-02-22T15:20:04Z

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 · 2019-02-22T21:17:21Z

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.

README.md

include/CLI/Error.hpp

henryiii · 2019-02-22T21:45:25Z

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 · 2019-02-22T21:58:01Z

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

phlptp · 2019-02-22T22:01:23Z

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 · 2019-02-22T22:08:22Z

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 · 2019-02-22T22:10:04Z

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

phlptp · 2019-02-22T22:16:41Z

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 · 2019-02-22T22:17:46Z

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

henryiii · 2019-02-22T22:33:45Z

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 · 2019-02-22T22:37:00Z

Did not know that

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

add some comments in readME about performance move operator[] to return const Option * Apply suggestions from code review Co-Authored-By: phlptp <top1@llnl.gov> update readme and add some IniTests and fix a bug from the tests add_flag_callback add a few tests to capture the different paths fix incorrectly updated CMAKE file, and add some subcommand test for option finding add disable_flag_override and work out some kinks in the find option functions add some more tests and fix a few bugs in as<> function for options Allow general flag types and default values, add shortcut notation for retrieving values

phlptp · 2019-02-23T00:00:22Z

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 · 2019-02-23T01:10:18Z

I think this is ready.

phlptp mentioned this pull request Feb 20, 2019

[WIP] Generalized flag values #229

Closed

phlptp force-pushed the general_flag_values branch from 9e7ccd4 to abc972c Compare February 20, 2019 17:08

phlptp force-pushed the general_flag_values branch from b19b0a3 to 8b03ef9 Compare February 20, 2019 20:10

henryiii reviewed Feb 20, 2019

View reviewed changes

examples/CMakeLists.txt Outdated Show resolved Hide resolved

include/CLI/App.hpp Show resolved Hide resolved

include/CLI/App.hpp Show resolved Hide resolved

include/CLI/App.hpp Outdated Show resolved Hide resolved

include/CLI/App.hpp Outdated Show resolved Hide resolved

phlptp force-pushed the general_flag_values branch from 8b03ef9 to 293d0b6 Compare February 21, 2019 15:26

phlptp mentioned this pull request Feb 22, 2019

Proper way to get path to the config file that was parsed #236

Closed

henryiii reviewed Feb 22, 2019

View reviewed changes

phlptp force-pushed the general_flag_values branch from c6ec129 to a733d3d Compare February 22, 2019 22:13

phlptp force-pushed the general_flag_values branch from a733d3d to 700db24 Compare February 22, 2019 23:56

henryiii merged commit ed5cd89 into CLIUtils:master Feb 23, 2019

phlptp deleted the general_flag_values branch February 23, 2019 12:06

henryiii mentioned this pull request Feb 28, 2019

Flag summing #243

Open

henryiii added this to the v1.8 milestone Feb 28, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow general flag types #235

Allow general flag types #235

phlptp commented Feb 20, 2019

codecov bot commented Feb 20, 2019 •

edited

Loading

codecov bot commented Feb 20, 2019

phlptp commented Feb 20, 2019

henryiii commented Feb 20, 2019

phlptp commented Feb 21, 2019 •

edited

Loading

phlptp commented Feb 22, 2019

henryiii commented Feb 22, 2019

henryiii commented Feb 22, 2019 •

edited

Loading

henryiii commented Feb 22, 2019

phlptp commented Feb 22, 2019

henryiii commented Feb 22, 2019

henryiii commented Feb 22, 2019

phlptp commented Feb 22, 2019

phlptp commented Feb 22, 2019

henryiii commented Feb 22, 2019 •

edited

Loading

henryiii commented Feb 22, 2019

phlptp commented Feb 23, 2019

phlptp commented Feb 23, 2019

Allow general flag types #235

Allow general flag types #235

Conversation

phlptp commented Feb 20, 2019

codecov bot commented Feb 20, 2019 • edited Loading

Codecov Report

codecov bot commented Feb 20, 2019

Codecov Report

phlptp commented Feb 20, 2019

henryiii commented Feb 20, 2019

phlptp commented Feb 21, 2019 • edited Loading

phlptp commented Feb 22, 2019

henryiii commented Feb 22, 2019

henryiii commented Feb 22, 2019 • edited Loading

henryiii commented Feb 22, 2019

phlptp commented Feb 22, 2019

henryiii commented Feb 22, 2019

henryiii commented Feb 22, 2019

phlptp commented Feb 22, 2019

phlptp commented Feb 22, 2019

henryiii commented Feb 22, 2019 • edited Loading

henryiii commented Feb 22, 2019

phlptp commented Feb 23, 2019

phlptp commented Feb 23, 2019

codecov bot commented Feb 20, 2019 •

edited

Loading

phlptp commented Feb 21, 2019 •

edited

Loading

henryiii commented Feb 22, 2019 •

edited

Loading

henryiii commented Feb 22, 2019 •

edited

Loading