app-template: add ability to specify options_description to be used for --help #2421
Conversation
…or --help Hiding options from --help output is useful when deprecating options. When using boost::program_options, the way to hide options from display is to provide an alternative options_description object and print that. So this patch adds a way to provide a help-specific options_description. While this entangles us even more with boost::program_options, I don't see a simple alternative.
|
User: scylladb/scylladb#20480 |
| @@ -129,13 +129,15 @@ private: | |||
| std::shared_ptr<smp> _smp; | |||
| seastar_options _opts; | |||
| boost::program_options::options_description _app_opts; | |||
| boost::program_options::options_description* _app_visible_opts_for_help = nullptr; | |||
There was a problem hiding this comment.
i'd suggest use &_app_opts as the default value of _app_visible_opts_for_help, like:
boost::program_options::options_description* _app_visible_opts_for_help = &_app_opts;this would help with the readability -- it's clear that we use _app_opts by default, when handling --help.
| _app_visible_opts_for_help = opts; | ||
| if (_app_visible_opts_for_help) { | ||
| add_extra_options(*_app_visible_opts_for_help); | ||
| } | ||
| } |
There was a problem hiding this comment.
could be more explicit, like:
if (opts) {
add_extra_options(*opts);
_app_visible_opts_for_help = opts;
}this way, it's clear that we mutate the pointee of opts, and it's a noop, if opts is nullptr. but i'd suggest accept a reference instead. like
set_app_visible_options_for_help(boost::program_options::options_description& opts)as, why would the caller want to pass a nullptr for a noop?
| @@ -153,6 +155,7 @@ public: | |||
| boost::program_options::options_description& get_options_description(); | |||
| boost::program_options::options_description& get_conf_file_options_description(); | |||
| boost::program_options::options_description_easy_init add_options(); | |||
| void set_app_visible_options_for_help(boost::program_options::options_description* opts); | |||
There was a problem hiding this comment.
if we really want to accept nullptr as a valid input, probably should add a doxygen comment to explain what nullptr is for. and, mutating the value passed by caller and expect that caller to keep the referenced object alive might also warrant a comment.
still i think a reference is a better fit in this case.
| @@ -211,7 +223,8 @@ app_template::run_deprecated(int ac, char ** av, std::function<void ()>&& func) | |||
| if (!_opts.description.empty()) { | |||
| std::cout << _opts.description << "\n"; | |||
| } | |||
| std::cout << _app_opts << "\n"; | |||
| auto& which_opts = _app_visible_opts_for_help ? *_app_visible_opts_for_help : _app_opts; | |||
There was a problem hiding this comment.
if we use &_app_opts as the default value of _app_visible_opts_for_help. we could simplify this part like:
assert(_app_visible_opts_for_help);
std::cout << *_app_visible_opts_for_help << "\n";
| @@ -109,12 +100,33 @@ app_template::app_template(app_template::config cfg) | |||
| { | |||
| } | |||
|
|
|||
| void | |||
| app_template::add_extra_options(boost::program_options::options_description& opts) { | |||
There was a problem hiding this comment.
instead of mutating the parameter, i'd suggest returning a boost::program_options::options_description. more readable this way, IMHO. BTW, could make this function static, if we don't want to mark it with the const specifier.
Hiding options from --help output is useful when deprecating options.
When using boost::program_options, the way to hide options from display is to provide an alternative options_description object and print that. So this patch adds a way to provide a help-specific options_description.
While this entangles us even more with boost::program_options, I don't see a simple alternative.