Adding map support to IsMember

[henryiii]

CLI::IsMember now supports maps. Example with an enum:

std::map<std::string, Level> map = {{"High", Level::High},
                                    {"Medium", Level::Medium},
                                    {"Low", Level::Low}};
Level level;
app.add_option("-l,--level", level, "Level settings")
    ->required()
    ->transform(CLI::IsMember(map, CLI::ignore_case)
              | CLI::IsMember({Level::High, Level::Medium, Level::Low}));

And, on the command line:

$ ./examples/enum -h                                                                                                                                                         Tue Feb 19 23:27:48 2019
Usage: ./examples/enum [OPTIONS]

Options:
  -h,--help                   Print this help message and exit
  -l,--level {High,Medium,Low} REQUIRED
                              Level settings

$ ./examples/enum -l high 
Enum received: 0

$ ./examples/enum -l low
Enum received: 2

$ ./examples/enum -l 1 
Enum received: 1

$ ./examples/enum -l 5
--level: 5 not in {High,Medium,Low} | 5 not in {0,1,2}
Run with --help for more information.

Changes:

• Containers of pairs convert from first (matched) to second (result) - this includes maps and the like
• Dropped "TEXT in" part of type name - should be clear enough
• Validator combinations are a bit better, and support functional type names
• Validators can be passed to ->transform now; ->check validators cannot modify the parse

Todo:

• Flip the order of maps
• Generalize to containers of pairs
• Check to see if type_name setting on option can override functional type names
• Add test with matching types
• Add to docs
• Add a test for transform with tname instead of only tname_function
• See if passing multiple functions to IsMember can avoid requiring string(string) form (might be in another PR)

[codecov]

Codecov Report

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

@@          Coverage Diff          @@
##           master   #228   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files          12     12           
  Lines        2057   2069   +12     
=====================================
+ Hits         2057   2069   +12

Impacted Files	Coverage Δ
include/CLI/Validators.hpp	100% <100%> (ø)	⬆️
include/CLI/TypeTools.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 5e0bb1c...5d4474d. Read the comment docs.

[codecov]

Codecov Report

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

@@          Coverage Diff          @@
##           master   #228   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files          12     12           
  Lines        2057   2087   +30     
=====================================
+ Hits         2057   2087   +30

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/Validators.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 5e0bb1c...48ec60d. Read the comment docs.

[henryiii]

Open question: Which direction should a map go? Currently values are matched (->second), and keys are returned (->first), but the other direction, while maybe less natural, makes more sense. You can get non-unique outputs, but non-unique input doesn't really make sense.

So currently:

{1: 'one',
 2: 'two}

If flipped:

{'one': 1,
 'two': 2,
 'uno': 1}

[phlptp]

In #229 I was playing with translate functions. I was using std::vector<std:pair<std::string, X>> as the basic type, as I didn't want to add maps but it would make sense to if IsMember supports them. In the context of translate the natural ordering would be {from, to} especially since there may be many strings that go to the same final element.

Maps are kind of interesting in that you might want to actually use the fast searching capabilities of a map. I could easily picture a case where you want to make sure an argument has a value in a hash table that is potentially large.

I would argue that isMember should not be modifying values. In the limited context of the functions I think it makes sense to match the member that is actually in the set, but with a map you are talking about much more significant transformations, and you add a requirement that the value part of the map must be convertible, whereas it is quite reasonable that you might want to search a structure that has a complex class as a value that you don't want translated. My instinct would be in the case of a map you don't want any requirement on the value portion of the map, since isMember should not be doing any translation other than in the presence of filter_functions making sure the actual value is a Member of original set. Any translation should be left to something that make that functionality much clearer.

[henryiii]

IsMember already has to modify the parsing, since things like ignore_case let you match against strings not technically in the set, but a user probably wants to see which item was matched against.

As you can see by the fact almost no code was added here, just minor type adaptors, this is exactly what IsMember was already doing; using a map (I can generalize to any container of pairs) just allows a user to control what happens. I prefer one flexible implementation over large numbers of different tools that all need extensive testing, etc. And this should be better for compile time, etc.

Just a thought I'll look into: how about check and transform both taking Validators, and only transform allowing the modification aspect to work? Then it would be more clear. We can also think about a rename for IsMember, too. Choices, for example?

[henryiii]

Both are now supported, and there is a difference between:

option->check(CLI::IsMember(...));
option->transform(CLI::IsMember(...));

Only the second version will modify the parse result, allowing a user to choose if they want the original string or the matched string. This is very nice for things like CLI::ignore_case, because you might want the original string, or you might want the one that was in the set for easy matching. If you use vectors of pairs or maps (both supported), the transform one is usually what you want, since you can use it to convert string matches into enums or integers.

I've flipped the order for maps/pairs, as well.

[henryiii]

Should the IsMember name be changed?

[henryiii]

This partially addresses #12, but I think @phlptp has one more improvement as well. @phlptp, can you take a look and see if you think this is ready to be merged? There's still the name issue; IsMember reads well for sets but maybe less so for maps (though since it is written ->transform(CLI::IsMember, that helps).

[phlptp]

I like where this is headed. The idea that check doesn't modify I think fits better, but it does add another layer of indirection, so are you ending up with what could easily be 3 layers of std::function calls. The naming on translate(IsMember) doesn't read well yet, not sure how to do it differently though right now, But I do think in a few more iterations this could be nailed down into a pretty solid interface. The best path forward might be to merge this and a few of the other PR's and see where things are at, as I am pretty sure they interact and could build on eachother.

[phlptp]

I think there will be a few more iterations on this idea before it settles down, but this is a good step and basis for further tweaks.