Adar Dembo has posted comments on this change.

Change subject: tool: better handling for positional arguments
......................................................................


Patch Set 1:

(11 comments)

> Sure wish there was some test coverage on these tools...

Alright, you got me. I'll add some in another patch.

 > There is a Go library which allows to implement rich CLI tools.  If
 > using the library it's really easy to document the
 > commands/sub-commands/parameters and it provides tab-completion
 > support for shell as an additional feature.  It might be useful to
 > take a look at that:
 > https://www.progville.com/go/cli-go-better-command-line-applications-go/
 > https://github.com/urfave/cli
 > 
 > There is even more powerful library named Cobra:
 > https://github.com/spf13/cobra
 > 
 > We implemented very decent command-line tool using codegangsta/cli
 > library in one of prior projects.
 > 
 > Also, if talking about command-line tools: given the abundance of
 > CLI libraries, it might make sense to consider an option to
 > implement those in Go.  It's really fast and simple in development
 > and seems to be the perfect language choice for that sort of
 > things.

Thanks for pointing me at those. I took a quick look at cli, and it's 
encouraging to see that it uses largely the same patterns that this tool does: 
actions nested under verbs, each of which is mapped to a function.

As for writing the tool in Go, Dan asked me the same question (but about 
Python). I wanted to use C++ because I wanted to avoid introducing a runtime 
dependency (this is mostly a knock against Python, since Go programs are 
statically linked), and I wanted to make it super easy to call into our 
existing C++ code.

 > Also, if it makes sense to exercise bash completion to in
 > displaying available sub-commands, it would also make sense to
 > check that.  Basically, it's about outputting the available
 > sub-commands if given --enable-bash-completion option at any level.

Didn't know this. Am I understanding you correctly: if a program responds to 
--enable-bash-completion with a list of verbs, then bash will invoke that 
behind the user's back when they try to tab complete? That is, if I run "kudu 
<TAB>", bash will try to run "kudu --enable-bash-completion" to figure out what 
the possible completions are?

http://gerrit.cloudera.org:8080/#/c/4013/1/src/kudu/tools/tool_action.cc
File src/kudu/tools/tool_action.cc:

PS1, Line 54: int
> Nit: would you mind to use string::size_type instead of int for first_dash_
Done


PS1, Line 164: int
> string::size_type ?
Done


http://gerrit.cloudera.org:8080/#/c/4013/1/src/kudu/tools/tool_action.h
File src/kudu/tools/tool_action.h:

Line 21: #include <glog/logging.h>
> Nit: if I'm not mistaken, by the code style the boost and glog headers shou
I've seen us go both ways, but I'll adhere to the more strict style as you 
suggest.


PS1, Line 135: marshaled
> I think it's clearer to just say 'put into the map'.
Done


PS1, Line 149: ActionArg
> the name sounds like it's a descriptor for one argument, whereas really it'
Done


Line 150:   struct Arg {
> I think this being a nested struct is a bit confusing, because, best I can 
But it is part of the API, at least in a manner of speaking: the parser 
accesses 'required' and 'variadic' so it needs to know the layout of Arg in 
order to do so.

Are you suggesting I pull it out into the top-level, as "ActionArg" or some 
such?


Line 162:   std::vector<std::string> optional;
> Does it make sense to have description for an optional parameter as well?  
Optional parameters are directly mapped to gflags, and the parameter 
description is part of the gflag itself.

I'll add a comment for this subtlety.


PS1, Line 187:  All remaining arguments on the command line will be joined (via 
single
             :   // space delimiter) to construct the parameter value.
> I find this odd vs an array.. otherwise you couldn't use this to pass a lis
I had three options:
1. Join the arguments together so that all command line arguments could be 
presented via the single map<string,string> at runtime.
2. Keep them separate (as a vector<string> or equivalent) and present the 
variadic args separately from the map.
3. Use boost::variant or something equivalently magical so that the arguments 
are presented as a single map<string,magic>.

I went with option #1 because I thought having all the args in one place made 
things net simpler. Would you rather option #2?


PS1, Line 191: Required
> Why are variadic params required?
I suppose they could be optional, but for the one concrete use case I had (list 
of peers) I thought we should enforce that at least one peer exists. Then I 
thought, if an action needs an optional variadic parameter, it could achieve 
that with a comma-delimited list passed via gflag.


Line 196:   // This parameter will be parsed as a gflag, and thus can be 
provided by the
> not clear what "parsed as a gflag" means. Is 'param' the _name_ of a gflag?
Yes, param is just the name of the gflag.

I'll admit relying on gflags for optional parameters is a little funky, but it 
does the job pretty well, as gflags handles the complexity of parsing dashed 
and double-dashed arguments anywhere in the command line. If an unspecified 
flag is passed here, the tool will crash (see the implementation).

I'll beef up the comments to explain this.


http://gerrit.cloudera.org:8080/#/c/4013/1/src/kudu/tools/tool_main.cc
File src/kudu/tools/tool_main.cc:

Line 60:   const ActionArgDescriptor* args = &action->args();
> What are benefits of using a pointer instead of a reference here?  Is it be
There's no operator overloading here; it was a purely stylistic choice. I'll 
change it.


-- 
To view, visit http://gerrit.cloudera.org:8080/4013
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ic6aa517ccf1915e4d106bcc5f80a3abfeae03271
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <a...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <a...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <aser...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <d...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <t...@apache.org>
Gerrit-HasComments: Yes

Reply via email to