CLI::IsMember #222

henryiii · 2019-02-08T14:42:08Z

This implements CLI::IsMember, and deprecates the default add_set* options . This fixes the set lifetime and mutability issues in #170 and makes sets easy to modify after adding if you use a (shared) pointer. It also combines case insensitivity and underscore ignoring into validators, without the massive number of methods on App needed now. You can use any iterable that supports std::begin, std::end, and has ::value_type, allowing a vector (for example) to be chosen to keep definition order - first item matches.

New features:

Single new validator, no longer a massive list of functions in App
Shared pointers supported, clear pointer vs. copy semantics
Combinable custom filter functions
All types support filter functions
Other containers, including ordered containers allowed

Changes:

The error for not being found in a set is now CLI::ValidationError
All add_set* functions use the new backend, and are deprecated, planned removal of most of them in 1.9, and all of them after that. mutable versions may be removed even in 1.8.

Ideas:

A ->choices(...) shortcut could be added, but the syntax is still up for debate, so not in this PR. Feel free to make suggestions.

Example of use:

// Simple sets
app.add_option("--name", name)->check(CLI::IsMember({"tom", "harry"}));

// Sets can be modified later. `ignore_case` and `ignore_underscores` is addable.
std::set<std::string> names = {"tom", "harry"};
app.add_option("--name", name)->check(CLI::IsMember(&names, CLI::ignore_case));
names->insert("john");

// Shared pointers are supported, and different types. And ordered containers.
auto vals = std::make_shared<std::vector<int>>({1,2});
app.add_option("--val", val)->check(CLI::IsMember(&vals));
names->push_back(3);

You can use std::shared_ptr (that's internally what's used if you don't give a pointer-like). You can list CLI::ignore_case or CLI::ignore_underscore (or any other function that will be applied to both sides).

Still to do:

Deprecate old add_set* methods (can't be done on all of them because some are templates.)
Provide new versions of all the tests (coexist for a little while)
Think about combined name + number style options (might not be part of this PR)
Add test with non-string function, like abs
Needs docs and examples
Fix ignore_underscore (no ending s) in the README.

henryiii · 2019-02-08T14:43:04Z

@phlptp There are also some warnings about unsigned vs. signed comparison fixed - though it looks like you may have already fixed them in your PR, that's fine (that will probably be done before this one by quite a bit). Feedback on the design welcome. :)

include/CLI/App.hpp

include/CLI/Set.hpp

phlptp · 2019-02-11T18:28:30Z

Just kind of poking around at things, but I was wondering if it might make sense to have the set member checking run through a Validator instead of in the lambda function. Then whatever add_set functions in the App class are just wrappers for add_option, and a ->check(CLI::IsMember(CLI::set)) Could even add CLI::IsNotMember(CLI::set) quite easily, then those Validators could have overloads for set or shared_ptr to a set. or chain set unions or whatever else you can do with Validators. It is a little bigger API change but it would probably clear out some additional code from App.hpp. Just a thought.

henryiii · 2019-02-12T08:29:09Z

That could work! It maps what you are doing (validating an option is in a set) much better than a special add function. That might even work around the need to have a mutable set at all, since you can add the set later on after creation. (Changing it would not be easy though, if you really needed it changed for multiple parses)

phlptp · 2019-02-12T16:39:45Z

I think in the case of reparsing, such as a terminal program or script interpreter, having a mutable set be a little more directly modifiable is probably useful, since you could very well change it between parses or even inside the callbacks themselves. Then I think supporting a shared_ptr to a set in an overload of the approvpriate Validator would make things straightforward.

codecov · 2019-02-15T13:25:16Z

Codecov Report

Merging #222 into master will decrease coverage by 0.34%.
The diff coverage is 75.86%.

@@            Coverage Diff             @@
##           master     #222      +/-   ##
==========================================
- Coverage     100%   99.65%   -0.35%     
==========================================
  Files          12       13       +1     
  Lines        2133     2054      -79     
==========================================
- Hits         2133     2047      -86     
- Misses          0        7       +7

Impacted Files	Coverage Δ
include/CLI/Set.hpp	`70.58% <70.58%> (ø)`
include/CLI/App.hpp	`99.79% <83.33%> (-0.21%)`	⬇️
include/CLI/StringTools.hpp	`100% <0%> (ø)`	⬆️
include/CLI/Option.hpp	`100% <0%> (ø)`	⬆️
include/CLI/Formatter.hpp	`100% <0%> (ø)`	⬆️
include/CLI/TypeTools.hpp	`100% <0%> (ø)`	⬆️
include/CLI/Split.hpp	`100% <0%> (ø)`	⬆️
include/CLI/ConfigFwd.hpp	`100% <0%> (ø)`	⬆️

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 3d7de7d...ca03b06. Read the comment docs.

codecov · 2019-02-15T13:25:17Z

Codecov Report

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

@@          Coverage Diff          @@
##           master   #222   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files          12     12           
  Lines        2133   2057   -76     
=====================================
- Hits         2133   2057   -76

Impacted Files	Coverage Δ
include/CLI/Timer.hpp	`100% <ø> (ø)`	⬆️
include/CLI/StringTools.hpp	`100% <ø> (ø)`	⬆️
include/CLI/TypeTools.hpp	`100% <ø> (ø)`	⬆️
include/CLI/Validators.hpp	`100% <100%> (ø)`	⬆️
include/CLI/Option.hpp	`100% <100%> (ø)`	⬆️
include/CLI/App.hpp	`100% <100%> (ø)`	⬆️
include/CLI/FormatterFwd.hpp	`100% <100%> (ø)`	⬆️
include/CLI/Error.hpp	`100% <0%> (ø)`	⬆️

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 3d7de7d...c06e3aa. Read the comment docs.

henryiii · 2019-02-15T13:30:56Z

For posterity, here's the old plan. PR now completely rewritten based on @phlptp's idea.

This is the start of CLI::Set. This fixes the set lifetime and mutability issues #170 and makes sets easy to modify after adding. It also combines case insensitivity and underscore ignoring into one package, without the massive number of methods on App needed now.

In the draft:

The name of the method is add_cli_set instead of add_set while both implementations coexist
The defaulted method is missing
No solution yet for enums and numerical matching to sets (maybe provide T param)
Try using std::shared_ptr as a way to pass in
Needs lots of tests
Needs to become the backend for the deprecated methods
Needs docs and examples

And, of course, it needs feedback.

This should give an idea of some ways to use:

app.add_cli_set("--name", name, {"tom", "harry"});

CLI::Set names("tom", "harry");
names->case_insensitive();
app.add_cli_set("--name", name, names);
names.insert("john");

phlptp · 2019-02-15T16:38:20Z

@henryiii I was tinkering with a few methods of making flags a little more generalized by extending the default false syntax a little. I think it will address #123 and #124. I want to try it out in a real application to try to work out any issues. I have a couple situations where the combination of the recent features will hopefully be able to collapse a bunch of different options into a single one. I want to wait until the set is a little more developed to make a PR on it though as I think it will interact nicely with it.

henryiii · 2019-02-15T22:20:35Z

Sounds good. This is nearly ready, just needs more testing and docs. I've been battling with the older compilers to get this to pass under C++11. I've had to give up nicer template errors but I think the basic syntax is intact.

It might be nice to have a shortcut method for creating a simple set inline, so far it's pretty convoluted.

app->add_option("--set", value)->set(CLI::case_insensitive, "one", "two", "three");

henryiii · 2019-02-16T09:09:24Z

I don't think I can make such a shortcut syntax work on GCC 4.7, it can't tell std::function from anything else.

phlptp · 2019-02-16T13:09:56Z

I have had some luck in the past using std::is_assignable<std::function<...>,Args> to check if something is a std::function or can be converted to one. That only works if std::function<... > doesn't participate in the template type deduction though. I think that works with C++11. and use that as part of SFINAE to differentiate between function like objects and regular types

Use IsMember now Using IsMember as backend for Set Non-const validator backend Move set tests to set Clearer inits

Tighten up classes a bit for MSVC Check with GCC 4.8 too

Dropping more type safety for older compilers Shortcut string set

Making g++ 4.7 docker image happy Fix Clang tidy issue with last commit Adding one more shortcut, adding a couple of tests

henryiii · 2019-02-17T12:54:39Z

Okay, @phlptp, you can see what you think, that worked partially, but ran into the issue that std::function<std::string(std::string)> = "" seems to be valid in GCC 4.7 (okay on 4.8+). I've added an extra check for const char *, and eventually may drop GCC 4.7 support. Due to a bug in GCC 4.7, I am not sure the last version or two worked on a proper GCC 4.7 with GCC 4.7's stdlib. I still don't think many users are on 4.7, since there's no major LTS OS that has 4.7 that I know of (4.8, on the other hand, is RHEL/CentOS'7 main compiler so will be supported for many years).

So, current points for review / comments:

CLI::IsMember has three signatures, pointer to container, container, and initializer_list of strings. The pointer can be any copyable pointer, such as a shared pointer. The containers can be any normal type, like a string or an int. const char * is not supported. Should I keep the initializer list shortcut?

opt->check(CLI::IsMember({"Inline", "set", "only", "supports", "strings"}, CLI::ignore_case));

Option->choices("choice1", "choice2", CLI::ignore_case) lets you quickly make a set inline from options. This one only supports strings. Modifier functions must come last. Is this the most useful sort of shortcut? I could also just allow ->choices(...) to be identical to ->check(CLI::IsMember(...)). This would allow it to also take sets and things, but would require extra {} around inline choices.

opt->choices({"Inline", "set", "only", "supports", "strings"}, CLI::ignore_case);
// vs.
opt->choices("Inline", "set", "only", "supports", "strings", CLI::ignore_case); // current version in code

No check for uniqueness is done. You can have a std::vector with duplicate entries, and the first one it finds will be matched. I could add a check, but I don't think it is that important?

Any other suggestions or review from anyone appreciated.

include/CLI/Validators.hpp

phlptp · 2019-02-17T14:19:04Z

My experience with gcc 4.7 and 4.8 is limited, Most of the projects I work on are currently at gcc 4.9+ to enable some of the C++14 stuff and we are starting to think about dropping 4.9 support. I would say you might consider tagging a 1.8 once things are settled a little then dropping 4.7 support in a 2.0 then you can make some bigger breaking changes if you want to.

phlptp · 2019-02-17T14:20:54Z

I don't think the uniqueness check is that important, In most cases you are looking a just a small set of options and most users won't have duplicates.

phlptp · 2019-02-17T17:51:09Z

I think it comes down to a stylistic choice. You could also consider dropping the choice member function completely. From a code readability ->check(CLI::IsMember(...)) is going to be pretty clear as to what it is doing (but slightly more verbose). Where as ->choices(...) is not bad but it is not as clear and adds more code that needs to maintained and managed and documented all to say it does the same thing as check(CLI::IsMember(...)).

henryiii · 2019-02-17T20:12:25Z

I like that idea - I'll drop choices then it can be readded based on usage later if needed.

include/CLI/Validators.hpp

henryiii · 2019-02-17T21:28:34Z

@SkyToGround, @kouchy, feel free to take a look too.

CMakeLists.txt

henryiii · 2019-02-18T09:47:10Z

Other types should now be supported for the initializer list. I've added a helper that corrects const char * to std::string; a user can further specialize if needed for custom type overrides.

You still need a C++ class for the container for ::value_type, so a plain array won't work.

SkyToGround · 2019-02-18T09:54:01Z

@henryiii I was about to write a comment about using enum class but seeing as you fixed that in your last commit I currently do not have any additional comments. 👍

include/CLI/App.hpp

henryiii · 2019-02-18T22:12:05Z

Will aim for a merge tomorrow if all looks good.

henryiii · 2019-02-19T07:47:50Z

Posting these unused snippets of code for posterity, since I'll squash and merge:

/// Allow a set to be quickly created
template <typename... Args> Option *choices(Args &&... args) {
    check(IsMember(std::forward<Args>(args)...));
    return this;
}
	
/// Allow a set to be quickly created (originally string only)
template <typename... Args> Option *choices(std::initializer_list<std::string> tmpset, Args &&... args) {
    std::vector<std::string> myset(tmpset);
    check(IsMember(myset, std::forward<Args>(args)...));
    return this;
}

And, the other form of the shortcut:

            /// Allow a set to be quickly created, strings followed optionally by functions.
	    ///
	    /// Start: Will always have std::string
	    template <typename... Args> Option *choices(std::string value, Args &&... args) {
	        std::vector<std::string> values = {value};
	        return choices(values, std::forward<Args>(args)...);
	    }
	
	  private:
	    /// Final function called - no function
	    Option *choices(std::vector<std::string> values) {
	        check(IsMember(values));
	        return this;
	    }
	
            /// Final function called - function present
	    template <typename T,
	              enable_if_t<std::is_assignable<std::function<std::string(std::string)>, T>::value &&
	                              !(std::is_same<T, const char *>::value || std::is_same<T, char *>::value),
	                          detail::enabler> = detail::dummy>
	    Option *choices(std::vector<std::string> values, T fn) {
	        check(IsMember(values, fn));
	        return this;
	    }
	
	    /// Append a string
	    template <typename T,
	              typename... Args,
	              enable_if_t<std::is_assignable<std::string, T>::value, detail::enabler> = detail::dummy>
	    Option *choices(std::vector<std::string> values, T value, Args &&... args) {
	        values.push_back(std::move(value));
	        return choices(values, std::forward<Args>(args)...);
	    }
	
	    /// Combine functions
	    template <typename... Args>
	    Option *choices(std::vector<std::string> values,
	                    std::function<std::string(std::string)> fn1,
	                    std::function<std::string(std::string)> fn2,
	                    Args &&... args) {
	        std::function<std::string(std::string)> fn = [fn1, fn2](std::string val) { return fn1(fn2(val)); };
	        return choices(values, fn, std::forward<Args>(args)...);
	    }

henryiii changed the title ~~WIP: CLI::Set~~ [WIP] CLI::Set Feb 8, 2019

phlptp reviewed Feb 8, 2019

View reviewed changes

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

phlptp reviewed Feb 8, 2019

View reviewed changes

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

phlptp reviewed Feb 9, 2019

View reviewed changes

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

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

henryiii force-pushed the henryiii-set branch from f4da463 to ca03b06 Compare February 15, 2019 13:25

henryiii force-pushed the henryiii-set branch from f425f31 to adad0de Compare February 16, 2019 16:29

henryiii added 6 commits February 17, 2019 07:55

Adding first draft of Sets

731de1d

Use IsMember now Using IsMember as backend for Set Non-const validator backend Move set tests to set Clearer inits

Drop shortcut

9bb5420

Tighten up classes a bit for MSVC Check with GCC 4.8 too

Simpler templates, but more of them

f262201

Dropping more type safety for older compilers Shortcut string set

Adding shortcut init

df590d5

Making g++ 4.7 docker image happy Fix Clang tidy issue with last commit Adding one more shortcut, adding a couple of tests

Dropping dual pointer versions of code

32ff88c

Smarter shortcut syntax

cd1c4ed

henryiii force-pushed the henryiii-set branch from 2f1925b to cd1c4ed Compare February 17, 2019 06:55

Adding slighly faster choices

c8508ad

phlptp reviewed Feb 17, 2019

View reviewed changes

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

Drop choices for now, adding some tests

438c8ed

phlptp reviewed Feb 17, 2019

View reviewed changes

include/CLI/Validators.hpp Show resolved Hide resolved

phlptp reviewed Feb 17, 2019

View reviewed changes

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

phlptp reviewed Feb 18, 2019

View reviewed changes

CMakeLists.txt Outdated Show resolved Hide resolved

ValidationError is now always the error from a validator

d93a0c1

henryiii force-pushed the henryiii-set branch from a458934 to d93a0c1 Compare February 18, 2019 07:59

Support for other types of initializer lists, including enums

ea35587

SkyToGround reviewed Feb 18, 2019

View reviewed changes

include/CLI/App.hpp Show resolved Hide resolved

Factor out type utilities, single version of IsMember code

6c2df1f

henryiii force-pushed the henryiii-set branch from 4a69602 to 6c2df1f Compare February 18, 2019 12:50

henryiii added 2 commits February 18, 2019 14:49

Adding a few tests for #224

263ab08

Minor cleanup for Validation Error

b9ef32d

henryiii force-pushed the henryiii-set branch from f8a8d7f to b9ef32d Compare February 18, 2019 14:57

henryiii changed the title ~~[WIP] CLI::IsMember~~ CLI::IsMember Feb 18, 2019

henryiii force-pushed the henryiii-set branch from bbdf90c to 5ace964 Compare February 18, 2019 21:28

Adding tests, moved deprecated tests

1ab1f91

henryiii force-pushed the henryiii-set branch from 5ace964 to 1ab1f91 Compare February 18, 2019 22:00

Docs updates

c06e3aa

henryiii merged commit c912381 into master Feb 19, 2019

henryiii deleted the henryiii-set branch February 19, 2019 09:16

elshize mentioned this pull request Feb 22, 2019

Upgrade CLI11 to 1.8 once released pisa-engine/pisa#118

Closed

henryiii modified the milestone: v1.8 Feb 28, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CLI::IsMember #222

CLI::IsMember #222

henryiii commented Feb 8, 2019 •

edited

Loading

henryiii commented Feb 8, 2019 •

edited

Loading

phlptp commented Feb 11, 2019

henryiii commented Feb 12, 2019

phlptp commented Feb 12, 2019

codecov bot commented Feb 15, 2019

codecov bot commented Feb 15, 2019 •

edited

Loading

henryiii commented Feb 15, 2019 •

edited

Loading

phlptp commented Feb 15, 2019

henryiii commented Feb 15, 2019

henryiii commented Feb 16, 2019

phlptp commented Feb 16, 2019

henryiii commented Feb 17, 2019

phlptp commented Feb 17, 2019

phlptp commented Feb 17, 2019

phlptp commented Feb 17, 2019

henryiii commented Feb 17, 2019

henryiii commented Feb 17, 2019

henryiii commented Feb 18, 2019 •

edited

Loading

SkyToGround commented Feb 18, 2019

henryiii commented Feb 18, 2019

henryiii commented Feb 19, 2019

CLI::IsMember #222

CLI::IsMember #222

Conversation

henryiii commented Feb 8, 2019 • edited Loading

New features:

Changes:

Ideas:

Example of use:

Still to do:

henryiii commented Feb 8, 2019 • edited Loading

phlptp commented Feb 11, 2019

henryiii commented Feb 12, 2019

phlptp commented Feb 12, 2019

codecov bot commented Feb 15, 2019

Codecov Report

codecov bot commented Feb 15, 2019 • edited Loading

Codecov Report

henryiii commented Feb 15, 2019 • edited Loading

phlptp commented Feb 15, 2019

henryiii commented Feb 15, 2019

henryiii commented Feb 16, 2019

phlptp commented Feb 16, 2019

henryiii commented Feb 17, 2019

phlptp commented Feb 17, 2019

phlptp commented Feb 17, 2019

phlptp commented Feb 17, 2019

henryiii commented Feb 17, 2019

henryiii commented Feb 17, 2019

henryiii commented Feb 18, 2019 • edited Loading

SkyToGround commented Feb 18, 2019

henryiii commented Feb 18, 2019

henryiii commented Feb 19, 2019

henryiii commented Feb 8, 2019 •

edited

Loading

henryiii commented Feb 8, 2019 •

edited

Loading

codecov bot commented Feb 15, 2019 •

edited

Loading

henryiii commented Feb 15, 2019 •

edited

Loading

henryiii commented Feb 18, 2019 •

edited

Loading