acelyc111 commented on code in PR #2040:
URL:
https://github.com/apache/incubator-pegasus/pull/2040#discussion_r1625320565
##########
src/shell/commands/cold_backup.cpp:
##########
@@ -157,15 +157,16 @@ bool ls_backup_policy(command_executor *e, shell_context
*sc, arguments args)
bool query_backup_policy(command_executor *e, shell_context *sc, arguments
args)
{
- const std::string query_backup_policy_help = "<policy_name>
[-b|--backup_info_cnt] [-j|--json]";
+ const std::string query_backup_policy_help =
+ "<-p|--policy_name> [-b|--backup_info_cnt] [-j|--json]";
argh::parser cmd(args.argc, args.argv,
argh::parser::PREFER_PARAM_FOR_UNREG_OPTION);
- RETURN_FALSE_IF_NOT(cmd.pos_args().size() > 1,
+ RETURN_FALSE_IF_NOT(cmd.pos_args().size() <= 1,
"invalid command, should be in the form of '{}'",
query_backup_policy_help);
int param_index = 1;
std::vector<std::string> policy_names;
- PARSE_STRS(policy_names);
+ PARSE_OPT_STRS(policy_names, " ", {"-p", "--policy_name"});
Review Comment:
Why pass a white space rather than en empty string?
##########
src/shell/commands/cold_backup.cpp:
##########
@@ -157,15 +157,16 @@ bool ls_backup_policy(command_executor *e, shell_context
*sc, arguments args)
bool query_backup_policy(command_executor *e, shell_context *sc, arguments
args)
{
- const std::string query_backup_policy_help = "<policy_name>
[-b|--backup_info_cnt] [-j|--json]";
+ const std::string query_backup_policy_help =
+ "<-p|--policy_name> [-b|--backup_info_cnt] [-j|--json]";
argh::parser cmd(args.argc, args.argv,
argh::parser::PREFER_PARAM_FOR_UNREG_OPTION);
- RETURN_FALSE_IF_NOT(cmd.pos_args().size() > 1,
+ RETURN_FALSE_IF_NOT(cmd.pos_args().size() <= 1,
Review Comment:
<-p|--policy_name> is required, but this judgement doesn't require this,
`query_backup_policy -b xxx` is also considered as valid, right?
##########
src/shell/command_helper.h:
##########
@@ -924,6 +924,24 @@ class aggregate_stats_calcs
}
\
} while (false)
+#define PARSE_OPT_STRS(container, def_val, ...)
\
+ do {
\
+ const auto param = cmd(__VA_ARGS__, (def_val)).str();
\
+ ::dsn::utils::split_args(param.c_str(), container, ',');
\
+ if (container.empty()) {
\
+ fmt::print(stderr,
\
Review Comment:
How about using `SHELL_PRINTLN_ERROR`?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
