Conversation
|
TODO
|
Codecov Report
@@ Coverage Diff @@
## master #358 +/- ##
=====================================
Coverage 100% 100%
=====================================
Files 12 12
Lines 3353 3396 +43
=====================================
+ Hits 3353 3396 +43
Continue to review full report at Codecov.
|
cbachhuber
left a comment
cbachhuber
left a comment
There was a problem hiding this comment.
Thank you for this addition! Looks very good already, I only have minor remarks.
| // deprecate an existing option and specify a recommended replacement | ||
| CLI::deprecate_option(opt2, "--not_deprecated"); | ||
|
|
||
| CLI11_PARSE(app, argc, argv); |
There was a problem hiding this comment.
Many other examples do a std::cout at the end, to show what is going on. Does that make sense here as well?
There was a problem hiding this comment.
Maybe something that prints out if the '--not_deprecated` or no options were passed, just so there is always guaranteed to be some output?
There was a problem hiding this comment.
Good suggestion, I like it!
include/CLI/App.hpp
Outdated
| ->type_name("RETIRED") | ||
| ->expected(0, 1) | ||
| ->default_str("RETIRED"); | ||
| Validator cv{[opt2](std::string &) { |
There was a problem hiding this comment.
Why is this object called cv? Could you find a more epressive name?
There was a problem hiding this comment.
custom validator, I will rename it
| } | ||
|
|
||
| /// Helper function to mark an option as deprecated | ||
| inline void deprecate_option(App &app, const std::string &option_name, const std::string &replacement = "") { |
There was a problem hiding this comment.
My IDE complains that this function is never used. Is my IDE wrong? Manually, I also can't find usages and wonder how you get 100% coverage.
There was a problem hiding this comment.
I thought I was using it one of the tests but I will double check that and make sure
include/CLI/App.hpp
Outdated
| /// Helper function to mark an option as retired | ||
| inline void retire_option(App *app, Option *opt) { | ||
| App temp; | ||
| auto ropt = temp.add_option(opt->get_name(false, true)) |
There was a problem hiding this comment.
Why do you need ropt in addition to opt? Consider a more descriptive name for this object
There was a problem hiding this comment.
We need to create a temporary psuedo copy of the option before destroying the original, and then copying the copy with the critical info. I will rename it something more descriptive though.
Co-Authored-By: Christoph Bachhuber <cbachhuber89@gmail.com>
…add more descriptions to deprecated options
cbachhuber
left a comment
cbachhuber
left a comment
There was a problem hiding this comment.
Looks good, thanks! 👍
|
Looks good to me, are you ready to merge? |
|
I think it is ready. |
3 participants
adds helper functions for retired and deprecated options
Fixes #275