cvd: Better help for load and start subcommands#2605
Conversation
New virtual functions for description and flags are added to the base command handler class, with default implementation since not all subcommands implement them yet. The default implementation of DetailedHelp will ensure all help messages will have the same style.
Not all command handlers will be able to use the default help implementation. They can still reuse some of the lower level utitlities by exposing those in a library.
By adding a marker to the string that the helper removes, but otherwise leaves the input string unmodified.
Use the default DetailedHelp implmentation for consistent formatting. Add more details to the help text.
The credential_source and project_id flags are parsed into respective string fields, only to later add them to the overrides list. This change makes the setter and getter of those flags interact with the overrides list directly. Assisted-by: Gemini:Next
Overriding the same property in the same instance is likely an error, so it's best to fail than produce unexpected results for the user.
| // This implementation will break each paragraph in lines not longer than 80 | ||
| // characters. |
There was a problem hiding this comment.
"This implementation" sounds like it refers to the default implementation of Description(), but that doesn't make sense: Description() doesn't have anything to do with formatting.
The comment is really referring to the default implementation of DetailedHelp, so the comment should say that instead.
| // Make sure the flags are in alphabetical order | ||
| std::sort(flags.begin(), flags.end(), | ||
| [](auto f1, auto f2) { return f1.Name() < f2.Name(); }); |
There was a problem hiding this comment.
Maybe worth defining operator< on flags, or at least a public function CompareName(const Flag&, const Flag&).
| } | ||
|
|
||
| std::string MarkAsRawText(std::string_view str) { | ||
| return std::string(kRawTextMark).append(str); |
| ) | ||
|
|
||
| cf_cc_library( | ||
| name = "help_helpers", |
| std::string config_path; | ||
| std::string credential_source; | ||
| std::string project_id; | ||
| std::map<std::string, std::string> overrides; |
There was a problem hiding this comment.
If this was std::map<std::string, std::string, std::less<void>>, it would be possible to use methods like operator[] or find with std::string_view arguments.
| // Test Setter | ||
| { |
There was a problem hiding this comment.
The tests added in this file look like they could be easily split up into multiple tests around these blocks. The only shared state is in the load_flags variable, and IMO the getter test would be easier to read if it didn't rely on the setter test running first.
| - "<absolute_path>")"), | ||
|
|
||
| "If the build value starts with \"@ab\", cvd will fetch the specified " | ||
| "Android build target from the Android build servers. By default it " |
There was a problem hiding this comment.
nit: two spaces between "Android" and "build"
| "cuttlefish/host/commands/cvd/cli/parser/load_config.proto.", | ||
|
|
||
| "While most config file properties are self explanatory, the build " | ||
|
|
||
| "properties (default_build, kernel.build, bootloader.build, etc) require " | ||
|
|
||
| "more explanation. These properties support two types of values:", |
| "The `cvd start` command applies to the instance group, not specific " | ||
| "instances because Cuttlefish instances in the same group must all be " | ||
| "started (and stopped) in unisom. The group to be started is chosen " | ||
| "using the standard selector flags.", | ||
| "Flags that modify individual instances accept a comma separated list of " | ||
| "values. If the number of values in one of these flags is less than the " | ||
| "number of instances, then the last instances in the group will default " | ||
| "to the first value in the list (as a result a single value provided " | ||
| "applies to all instances). The empty string is interpreted as a valid " | ||
| "value, to force a particular instance to use its intended default value " | ||
| "the special value \"unset\" must be given."}; |
There was a problem hiding this comment.
nit: maybe use a R"(...)" string here?
| Result<std::string> LoadConfigsCommand::DetailedHelp( | ||
| std::vector<std::string> LoadConfigsCommand::Description() const { | ||
| return { | ||
| "This command is an alias of `cvd create --config_file=<config_filepath> " |
There was a problem hiding this comment.
nit: maybe use R"(...)" strings even for the non-MarkAsRawText help strings?
2 participants
Added a default implementation of DetailedHelp that prints in the same format for all commands (unless they override it, which most still do):
Both
loadandstartmake use of that default implementation.Added more information and missing flags to both command's help output.
Some QoL improvements to the load command.