Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

To string and default option revamp #242

Merged
merged 3 commits into from
Apr 28, 2019
Merged

To string and default option revamp #242

merged 3 commits into from
Apr 28, 2019

Conversation

henryiii
Copy link
Collaborator

@henryiii henryiii commented Feb 28, 2019

This adds to_string, which automatically selects the best way to convert a type to a string, and returns an empty string if the type is unconvertable. This removes the two overloads for defaulted, and has the side effect that now capturing a default can be done later through the option, the way all other option settings (except name and variable) work. This was also implemented in ->capture_default_str(). Here's the before and after:

// Classic method (still works for now)
app.add_option("--opt", opt, "", true);

// New method
app.add_option("--opt", opt)
    ->capture_default_str();

The bool in the add_option constructor be deprecated (no warning in 1.8, warning in 1.9, removed in 2.0)? It was a workaround for missing technology. The description string stays (even though it can also be set later) since adding a description should be done 95% of the time, and it's clear what it is from looking at it. A random true at the end is not clear at all.

Remaining questions:

  • Is capture_default_str() the best name? default is a C++ keyword, so that's out. It's a bit long for something that used to be four chars (true).

Todo:

  • Add to readme
  • Convert tests to the new syntax, add some copies with the old one
  • Test Remove the delayed behavior of the default capture
  • Test the always_default_capture() setting

Note there is one minor drawback. If a type supports streaming into a stream and has a declaration, but does not have the streaming definition available in the included header (like in Boost::Optional), you will need to add the streaming header to add options. Since adding the streaming enhances CLI11, I think this is an acceptable drawback. Unstreamable types are supported (and tested now).

This closes #241.

Features:

  • Adds ->capture_default_str(), which stores the current value of the attached variable and will show it in the help string.
  • Adds ->always_capture_default() (INHERITABLE), which will cause the default to always be captured. Really only useful on the template option. Includes get_ always_capture_default().
  • Using always_capture_default or the "true" option captures the default string at the point of definition, rather than later.
  • default_function has been added, and is settable.
  • get_defaultval renamed to get_default_str

@codecov
Copy link

codecov bot commented Feb 28, 2019

Codecov Report

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

Impacted file tree graph

@@          Coverage Diff          @@
##           master   #242   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files          12     12           
  Lines        2771   2781   +10     
=====================================
+ Hits         2771   2781   +10
Impacted Files Coverage Δ
include/CLI/Formatter.hpp 100% <100%> (ø) ⬆️
include/CLI/Config.hpp 100% <100%> (ø) ⬆️
include/CLI/Option.hpp 100% <100%> (ø) ⬆️
include/CLI/App.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 ca4bc6a...dbbd793. Read the comment docs.

@codecov
Copy link

codecov bot commented Feb 28, 2019

Codecov Report

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

Impacted file tree graph

@@          Coverage Diff          @@
##           master   #242   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files          12     12           
  Lines        2212   2206    -6     
=====================================
- Hits         2212   2206    -6
Impacted Files Coverage Δ
include/CLI/App.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 9f81385...d5c8a5f. Read the comment docs.

@henryiii henryiii changed the title [WIP] To string [WIP] To string and default option revamp Feb 28, 2019
@henryiii
Copy link
Collaborator Author

henryiii commented Feb 28, 2019

Open for comments. @phlptp, this is what I was referring to in #240. I based CLI::detail::to_string heavily on your function in #239, hopefully they could be combined.

@henryiii henryiii changed the title [WIP] To string and default option revamp To string and default option revamp Feb 28, 2019
@henryiii henryiii added this to the v1.8 milestone Feb 28, 2019
include/CLI/App.hpp Outdated Show resolved Hide resolved
@phlptp
Copy link
Collaborator

phlptp commented Feb 28, 2019

I agree the defaulted flag is not the most intuitive of arguments. If I understand it correctly turning that flag to true when defining an option specifies that whatever value I have currently in the value I loaded should be used as the default value and when the App is executed if that particular option is not used the default_val is loaded into the variable?

So use cases
When I am just using a single variable an one shot processing, I probably wouldn't use default as CLI11 shouldn't be messing with the variable so I would just stick the base value in that variable.

There may be times I would want a option/flag callback to be called regardless of whether the flag is specified or not. A way of making sure that happens with a default value would be nice, as well as turning that on or off.

If I am using CLI11 as a text program processor, then I might want variables reset to a default value each time, so CLI11 should replace the current value with the default if no option was specified.

So I think if I were redesigning it I would want a defaulted() toggle to change the behavior of replacement or callback execution with no arguments. And I would want a default(T val) function to specify the default later and change it if necessary. Along with appropriate getter functions.

With the default function, I would like it to be a template so it can just convert whatever type I give it whatever the internal representation in CLI11 is. In this case I would think if it can't convert a compile error is probably good, and silently doing a conversion is probably bad.

Taking the storage value and using it as the default seems to me it might a little too tricky, and then using that in some sort of generator function, I just have a little trouble seeing the use case.

@henryiii
Copy link
Collaborator Author

The "default value" is only a help printout. CLI11 never changes the value of the variable if it is not passed (for options, at least) - callbacks do not even run if the option is empty. Since the variable is just a plain variable, the programmer is responsible for initializing it if is to be used later. The only change for adding "true" (currently) or ->capture_default() in this PR is that you are telling CLI11 that it's okay to read the value from the variable and to print in the help. This is the way it works currently (before this PR)

int value = 2;

// If value is uninitialized, this is still valid
app.add_option("--opt", value, "A value");
// Help is:
// --opt=INT        A value

// If value is uninitialized, this is not valid (there is no good reason to do this)
app.add_option("--opt", value, "A value", false);
// Help is:
// --opt INT        A value

// The default value is added to the help
app.add_option("--opt", value, "A value", true);
// Help is:
// --opt INT=2      A value

In all cases, if the option is not passed, nothing is touched. And you can always set your own default string with ->defaut_str("2"), but this is not as handy as being able to request an automatic default string.

Adding a callback that would run to reset things to defaults is beyond the scope of this PR. I'm not sure CLI11 should ever meddle with defaults, since a programmer can just as easily and more transparently set the values back to defaults (they are just plain types after all). In that case, one might want the delayed default checking.

@henryiii
Copy link
Collaborator Author

I like shorter names for something (at least I) use often, but capture_default_str might be more descriptive. Maybe making it inheritable would fix the issue of the long name, at least for someone that always sets it.

@phlptp
Copy link
Collaborator

phlptp commented Feb 28, 2019

Ok, I haven't messed too much with the default stuff in our use cases, so I can't say I fully understood what was going on. I guess I can see how the capture by reference thing would work now.
I do like the few overloads in the App class, that simplifies things!

@henryiii henryiii force-pushed the henryiii-tostream branch 4 times, most recently from 759266d to bcdbd16 Compare March 3, 2019 15:53
@henryiii henryiii force-pushed the henryiii-tostream branch from bcdbd16 to 6e7535f Compare March 26, 2019 02:35
@henryiii henryiii force-pushed the henryiii-tostream branch from bb6b63f to 64fae40 Compare April 11, 2019 10:21
@dvj
Copy link
Contributor

dvj commented Apr 12, 2019

Even at the somewhat lengthy name of capture_default_str, it's not obvious that its function is to change the output of the help text. Another way to accomplish might be to have a help_text() function that the user could explicitly choose how the help text gets formatted, i.e.:
help_text(bool show_default_value, bool show_type, bool show_equal_sign, ...)

Good change though, however it gets worked out!

Using to_string instead

Switching to new backend

Moving to capture function for defaults

Rename capture_default + _str

defaultval -> default_str, added always_capture_default

Fix style
@henryiii henryiii force-pushed the henryiii-tostream branch from 64fae40 to e15ef57 Compare April 28, 2019 15:50
@henryiii henryiii force-pushed the henryiii-tostream branch from 89b0c63 to dbbd793 Compare April 28, 2019 18:20
@henryiii henryiii merged commit d818430 into master Apr 28, 2019
@henryiii henryiii deleted the henryiii-tostream branch April 28, 2019 20:44
@henryiii
Copy link
Collaborator Author

@djv, I don't think help_text(true, false, true) is any more helpful for a user, and makes your code much harder to read. I think this might be something that could be refactored or streamlined in the future; this is all really about help rather than functionality, so maybe there could be some separation. I'll be setting up a plan for 1.8, but feel free to test and suggest improvements!

@henryiii henryiii mentioned this pull request Apr 28, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Investigate update to default capture
3 participants