[WIP] Generalized flag values #229

phlptp · 2019-02-19T17:05:47Z

This PR will make flag more general in that they can handle any non-vector type and have default values of any type.

It adds better enum support

adds a transformer and checkedTransformer validators.

Also adds ["option_name"] syntax for getting options and as for querying results to be translated to any available type.

phlptp · 2019-02-19T17:10:03Z

@henryiii This what I have been tinkering with, Not sure you would want any or all of it. My target is making a transition from boost::program options in a complex layered setup a little easier. I also saw you were playing with some map stuff, which is related to some things I was tinkering with related to Transformer. I suspect you probably don't want both but they may be able to be merged.

Right now the PR has a number of merges from the recent set PR so that is why there is so many commits. I might be able to peel off some of them to make a few other PRs of specific components.

phlptp · 2019-02-19T17:23:27Z

This will also address Issues #12 , #123, and #124 I think

codecov · 2019-02-19T20:04:31Z

Codecov Report

Merging #229 into master will decrease coverage by 1.84%.
The diff coverage is 88.44%.

@@            Coverage Diff             @@
##           master     #229      +/-   ##
==========================================
- Coverage     100%   98.15%   -1.85%     
==========================================
  Files          12       12              
  Lines        2057     2228     +171     
==========================================
+ Hits         2057     2187     +130     
- Misses          0       41      +41

Impacted Files	Coverage Δ
include/CLI/StringTools.hpp	`100% <100%> (ø)`	⬆️
include/CLI/Split.hpp	`100% <100%> (ø)`	⬆️
include/CLI/TypeTools.hpp	`100% <100%> (ø)`	⬆️
include/CLI/ConfigFwd.hpp	`100% <100%> (ø)`	⬆️
include/CLI/Option.hpp	`94.18% <68.85%> (-5.82%)`	⬇️
include/CLI/Validators.hpp	`94.16% <83.14%> (-5.84%)`	⬇️
include/CLI/App.hpp	`99.31% <95.29%> (-0.69%)`	⬇️
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 5e0bb1c...1f26a61. Read the comment docs.

codecov · 2019-02-19T20:04:31Z

Codecov Report

Merging #229 into master will decrease coverage by 1.84%.
The diff coverage is 88.44%.

@@            Coverage Diff             @@
##           master     #229      +/-   ##
==========================================
- Coverage     100%   98.15%   -1.85%     
==========================================
  Files          12       12              
  Lines        2057     2228     +171     
==========================================
+ Hits         2057     2187     +130     
- Misses          0       41      +41

Impacted Files	Coverage Δ
include/CLI/StringTools.hpp	`100% <100%> (ø)`	⬆️
include/CLI/Split.hpp	`100% <100%> (ø)`	⬆️
include/CLI/TypeTools.hpp	`100% <100%> (ø)`	⬆️
include/CLI/ConfigFwd.hpp	`100% <100%> (ø)`	⬆️
include/CLI/Option.hpp	`94.18% <68.85%> (-5.82%)`	⬇️
include/CLI/Validators.hpp	`94.16% <83.14%> (-5.84%)`	⬇️
include/CLI/App.hpp	`99.31% <95.29%> (-0.69%)`	⬇️
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 5e0bb1c...6e12ea2. Read the comment docs.

henryiii · 2019-02-19T22:47:44Z

Needs a rebase or something equivalent. Sounds exciting. Wonder how it compares to #228. Edit: yes, as you mentioned in your comment.

* move toward generalized default flag values * collapse the add_flag functions with a common element, add some additional results output functions * clear up some additional shadow warnings, fix some of the templates, add a few more tests * add Transform object * add checkedTransform validator * add digit_args examples

…ed_flag_values

henryiii · 2019-02-20T08:18:45Z

It would be nice to at least split into two PRs, one for transformer and checkedTransformer validators. I think I'd like to compare with the IsMember validator. The other parts sound useful and could be evaluated and merged better on their own.

henryiii · 2019-02-20T09:03:33Z

include/CLI/App.hpp

-            throw IncorrectConstruction::PositionalFlag(flag_name);
+  private:
+    /// internal function for adding a flag
+    Option *add_flag_internal(std::string &flag_name, CLI::callback_t &fun, std::string &flag_description) {


This should start with an underscore, internal functions all start with underscore. (Python style)

henryiii · 2019-02-20T09:04:43Z

include/CLI/App.hpp

-            detail::sum_flag_vector(res, flag_count);
+            try {
+                detail::sum_flag_vector(res, flag_count);
+            } catch(const std::invalid_argument &) {


I don't think I've seen invalid_argument, nice.

henryiii · 2019-02-20T09:05:19Z

include/CLI/App.hpp

        opt->check(IsMember{options});
        return opt;
    }

-    /// Add set of options (No default, set can be changed afterwords - do not destroy the set) DEPRECATED
+    /// Add set of options (No default, set can be changed afterwards - do not destroy the set)


DEPRECATED, please :)

henryiii · 2019-02-20T09:07:30Z

include/CLI/App.hpp

-            op = get_option("--" + item.name);
-        } catch(const OptionNotFound &) {
+        Option *op = get_option_no_throw("--" + item.name);
+        if(op == nullptr) {


Nice, I like avoiding exceptions as control flow if possible.

henryiii · 2019-02-20T09:10:15Z

include/CLI/ConfigFwd.hpp

@@ -69,14 +69,10 @@ class Config {
    /// Convert a configuration into an app
    virtual std::vector<ConfigItem> from_config(std::istream &) const = 0;

-    /// Convert a flag to a bool representation
+    /// get a flag value


First char caps for docstrings and for comments alone on a line. Comments that follow code on the same line are not capitalized. Okay for now, but slightly easier if I don't have to correct it later.

henryiii · 2019-02-20T09:13:17Z

include/CLI/Option.hpp

@@ -330,6 +332,25 @@ class Option : public OptionBase<Option> {
        return this;
    }

+    /// Adds a transformation/validator with a built in type name
+    Option *transform(const Transformer &tform) {


I've applied a very similar idea in my PR, but without the strong typing. I could add typing so that a Validator could not be added to ->transform, but the ability to choose whether a tranformer actually transforms or not is really nice for a user, IMO. It also avoids duplication.

henryiii · 2019-02-20T09:17:49Z

include/CLI/Option.hpp

+    /// get the results as a particular type
+    template <typename T,
+              enable_if_t<!is_vector<T>::value && !std::is_const<T>::value, detail::enabler> = detail::dummy>
+    void results(T &output) const {


Interesting! I've been looking for a way to add types to Option for 2.0, but this partially (not fully) negates the need to do that. The tricky part about the 2.0 idea is that Option<T> would need to be stored in the shared/unique pointer array, and I was thinking it would need a reference or copy of T, but this might avoid that (at a cost per access).

Does this pick up the results of a transform? I assume so since they should be transformed in place?

Could this be broken out into a standalone function(s including overload)? Then it could be reused by the normal parse also, possibly? That is, this would become something like:

template<typename T> void results(T &output) { CLI::detail::results(output, results_, defaultval_, multi_option_policy_); }

And CLI::detail::results could be used in the parse function, too. Maybe not, just thinking. Also, it might be possible to make CLI::detail::results a struct, and it could have things like default number of arguments attached, and a user could provide specializations to add support for new types. (now well outside the scope of this PR, just thinking)

henryiii · 2019-02-20T09:27:00Z

include/CLI/StringTools.hpp

+
+/// output streaming for enumerations
+template <typename T, typename = typename std::enable_if<std::is_enum<T>::value>::type>
+std::ostream &operator<<(std::ostream &in, const T &level) {


Nice addition that I didn't think of. It might be a good idea to make it opt-in in some way though, so a user doesn't clash if they already have something similar. Like using namespace CLI::enums?

Fantastic, inside an exported namespace it works great, I'm adding this to my PR if you don't mind...

Feel free, It took a little while to figure out where to actually put this in the code so it was actually visible to all the functions that might need it. I pulled it from the enum.cpp example, so it would work with the transformer Validator.

henryiii · 2019-02-20T09:31:03Z

include/CLI/StringTools.hpp

+                                  const std::vector<std::string> names,
+                                  bool ignore_case = false,
+                                  bool ignore_underscore = false) {
+    auto it = std::end(names);
    if(ignore_case) {
        if(ignore_underscore) {


Would it make sense to have a vector of transform functions, and allow a user to add any transforms they want; something like opt->filter(CLI::ignore_underscore)->filter(CLI::ignore_case)? Vector would be part of default option, etc. Just a thought for later.

One thing I ran into was that translate functions should run before the actual validators so I had to make sure they were inserted at the front of the validator vector.
So I think it is kind of an interesting usability discussion of whether check() should be allowed to modify or not. On one had that differentiates check and translate but it disallows a CheckedTranslate since that would translate and make sure it was translated as a single unit. You would need a translate and a separate check.

henryiii · 2019-02-20T09:32:20Z

include/CLI/TypeTools.hpp

+/// print name for enumeration types
+template <typename T, enable_if_t<std::is_enum<T>::value, detail::enabler> = detail::dummy>
+constexpr const char *type_name() {
+    return "ENUM";


henryiii · 2019-02-20T09:33:04Z

include/CLI/Validators.hpp

+    /// create a validator that fails when a given validator succeeds
+    Validator operator!() const {
+        Validator newval;
+        newval.tname = "NOT " + tname;


You might want to support tname_function, too.

henryiii · 2019-02-20T09:36:10Z

tests/app_helper.hpp

@@ -41,15 +41,15 @@ class TempFile {
 };

 inline void put_env(std::string name, std::string value) {
-#ifdef _MSC_VER
+#ifdef _WIN32


I am not very familiar with non-MSVC compilers on Windows anymore, so never know what is best here. I'll assume that non-MSVC compilers are still able to access the Windows calls, rather than GNU counterparts? It would be nice to add a test to AppVeyor (or Azure) someday (not as part of this PR, though).

The issue was MSYS, which is mingw, deep in some of the libraries is uses some of the windows subsystems, so has most of the posix calls, but when you get down into environmental variables you use the windows calls, which I think was the case here.

phlptp · 2019-02-20T12:58:32Z

This might to much to adequately review, I will try to split some things up into different PR's, they were all interconnected but I think there are some distinct pieces that are are valid on their own.

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

I added a number of PR #230, #232, #233, #235. That try to parse out specific components of this PR. I haven't added one for the transform piece yet since I think #228 will modify it heavily.

phlptp · 2019-02-21T15:32:29Z

I am not going to fix this PR so it will be closed eventually, but am leaving in place for the moments since it contains a useful discussion at least until the PR's are merged/closed out.

phlptp · 2019-03-01T15:54:36Z

This will be closed when #239 is merged or closed

phlptp · 2019-03-02T13:45:37Z

All parts of this have now been merged

phlptp force-pushed the generalized_flag_values branch from 1f26a61 to 545999d Compare February 19, 2019 23:19

Merge remote-tracking branch 'remotes/original/master' into generaliz…

6e12ea2

…ed_flag_values

phlptp mentioned this pull request Feb 20, 2019

Adding map support to IsMember #228

Merged

6 tasks

henryiii reviewed Feb 20, 2019

View reviewed changes

phlptp mentioned this pull request Feb 20, 2019

Add a ! operator to Validators for checking a false condition #230

Merged

phlptp mentioned this pull request Feb 20, 2019

Allow general flag types #235

Merged

phlptp mentioned this pull request Feb 24, 2019

add transformer and checkedTransformer #239

Merged

phlptp closed this Mar 2, 2019

phlptp deleted the generalized_flag_values branch March 2, 2019 13:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] Generalized flag values #229

[WIP] Generalized flag values #229

phlptp commented Feb 19, 2019

phlptp commented Feb 19, 2019

phlptp commented Feb 19, 2019

codecov bot commented Feb 19, 2019

codecov bot commented Feb 19, 2019 •

edited

Loading

henryiii commented Feb 19, 2019 •

edited

Loading

henryiii commented Feb 20, 2019

henryiii Feb 20, 2019

henryiii Feb 20, 2019

henryiii Feb 20, 2019

henryiii Feb 20, 2019

henryiii Feb 20, 2019

henryiii Feb 20, 2019

henryiii Feb 20, 2019

henryiii Feb 20, 2019

henryiii Feb 20, 2019 •

edited

Loading

henryiii Feb 20, 2019

phlptp Feb 20, 2019

henryiii Feb 20, 2019 •

edited

Loading

phlptp Feb 20, 2019

henryiii Feb 20, 2019

henryiii Feb 20, 2019

henryiii Feb 20, 2019 •

edited

Loading

phlptp Feb 20, 2019

phlptp commented Feb 20, 2019

phlptp commented Feb 20, 2019

phlptp commented Feb 21, 2019

phlptp commented Mar 1, 2019

phlptp commented Mar 2, 2019

[WIP] Generalized flag values #229

[WIP] Generalized flag values #229

Conversation

phlptp commented Feb 19, 2019

phlptp commented Feb 19, 2019

phlptp commented Feb 19, 2019

codecov bot commented Feb 19, 2019

Codecov Report

codecov bot commented Feb 19, 2019 • edited Loading

Codecov Report

henryiii commented Feb 19, 2019 • edited Loading

henryiii commented Feb 20, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

henryiii Feb 20, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

henryiii Feb 20, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

henryiii Feb 20, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

phlptp commented Feb 20, 2019

phlptp commented Feb 20, 2019

phlptp commented Feb 21, 2019

phlptp commented Mar 1, 2019

phlptp commented Mar 2, 2019

codecov bot commented Feb 19, 2019 •

edited

Loading

henryiii commented Feb 19, 2019 •

edited

Loading

henryiii Feb 20, 2019 •

edited

Loading

henryiii Feb 20, 2019 •

edited

Loading

henryiii Feb 20, 2019 •

edited

Loading