[kudu-CR] tool: better handling for positional arguments
Todd Lipcon has submitted this change and it was merged. Change subject: tool: better handling for positional arguments .. tool: better handling for positional arguments This patch modifies Action to describe all of its argument requirements, not just gflags. As a result, we can improve usability in two major ways: 1. The parser itself can process positional arguments, leaving less work for individual actions. 2. The action help strings can describe positional arguments. Change-Id: Ic6aa517ccf1915e4d106bcc5f80a3abfeae03271 Reviewed-on: http://gerrit.cloudera.org:8080/4013 Tested-by: Kudu Jenkins Reviewed-by: Todd Lipcon --- M src/kudu/tools/tool_action.cc M src/kudu/tools/tool_action.h M src/kudu/tools/tool_action_fs.cc M src/kudu/tools/tool_action_tablet.cc M src/kudu/tools/tool_main.cc M src/kudu/util/subprocess.cc 6 files changed, 246 insertions(+), 134 deletions(-) Approvals: Todd Lipcon: Looks good to me, approved Kudu Jenkins: Verified -- To view, visit http://gerrit.cloudera.org:8080/4013 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: merged Gerrit-Change-Id: Ic6aa517ccf1915e4d106bcc5f80a3abfeae03271 Gerrit-PatchSet: 5 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon
[kudu-CR] tool: better handling for positional arguments
Todd Lipcon has posted comments on this change. Change subject: tool: better handling for positional arguments .. Patch Set 4: Code-Review+2 -- 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: 4 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: No
[kudu-CR] tool: better handling for positional arguments
Adar Dembo has posted comments on this change. Change subject: tool: better handling for positional arguments .. Patch Set 3: (1 comment) http://gerrit.cloudera.org:8080/#/c/4013/3/src/kudu/tools/tool_action.cc File src/kudu/tools/tool_action.cc: Line 145:const boost::optional>& variadic_args) const { > why an optional vector instead of just an empty vector inthe case that ther Done -- 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: 3 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: Yes
[kudu-CR] tool: better handling for positional arguments
Kudu Jenkins has posted comments on this change. Change subject: tool: better handling for positional arguments .. Patch Set 4: Build Started http://104.196.14.100/job/kudu-gerrit/2985/ -- 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: 4 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: No
[kudu-CR] tool: better handling for positional arguments
Hello Todd Lipcon, Alexey Serbin, Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/4013 to look at the new patch set (#4). Change subject: tool: better handling for positional arguments .. tool: better handling for positional arguments This patch modifies Action to describe all of its argument requirements, not just gflags. As a result, we can improve usability in two major ways: 1. The parser itself can process positional arguments, leaving less work for individual actions. 2. The action help strings can describe positional arguments. Change-Id: Ic6aa517ccf1915e4d106bcc5f80a3abfeae03271 --- M src/kudu/tools/tool_action.cc M src/kudu/tools/tool_action.h M src/kudu/tools/tool_action_fs.cc M src/kudu/tools/tool_action_tablet.cc M src/kudu/tools/tool_main.cc M src/kudu/util/subprocess.cc 6 files changed, 246 insertions(+), 134 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/13/4013/4 -- To view, visit http://gerrit.cloudera.org:8080/4013 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: Ic6aa517ccf1915e4d106bcc5f80a3abfeae03271 Gerrit-PatchSet: 4 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon
[kudu-CR] tool: better handling for positional arguments
Alexey Serbin 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 ", bash will try to run > "kudu --enable-bash-completion" to figure out what the possible > completions are? Yes, that's the idea. The bash can use different option. Actually, an additional .bashrc customization will be necessary. For details, please take at https://github.com/urfave/cli/blob/master/autocomplete/bash_autocomplete -- 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 Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: No
[kudu-CR] tool: better handling for positional arguments
Todd Lipcon has posted comments on this change. Change subject: tool: better handling for positional arguments .. Patch Set 3: (1 comment) http://gerrit.cloudera.org:8080/#/c/4013/3/src/kudu/tools/tool_action.cc File src/kudu/tools/tool_action.cc: Line 145:const boost::optional>& variadic_args) const { why an optional vector instead of just an empty vector inthe case that there are none? shouldn't the tool already know on its own whether it wanted them? -- 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: 3 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: Yes
[kudu-CR] tool: better handling for positional arguments
Kudu Jenkins has posted comments on this change. Change subject: tool: better handling for positional arguments .. Patch Set 3: Build Started http://104.196.14.100/job/kudu-gerrit/2981/ -- 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: 3 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: No
[kudu-CR] tool: better handling for positional arguments
Hello Alexey Serbin, Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/4013 to look at the new patch set (#3). Change subject: tool: better handling for positional arguments .. tool: better handling for positional arguments This patch modifies Action to describe all of its argument requirements, not just gflags. As a result, we can improve usability in two major ways: 1. The parser itself can process positional arguments, leaving less work for individual actions. 2. The action help strings can describe positional arguments. Change-Id: Ic6aa517ccf1915e4d106bcc5f80a3abfeae03271 --- M src/kudu/tools/tool_action.cc M src/kudu/tools/tool_action.h M src/kudu/tools/tool_action_fs.cc M src/kudu/tools/tool_action_tablet.cc M src/kudu/tools/tool_main.cc M src/kudu/util/subprocess.cc 6 files changed, 247 insertions(+), 134 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/13/4013/3 -- To view, visit http://gerrit.cloudera.org:8080/4013 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: Ic6aa517ccf1915e4d106bcc5f80a3abfeae03271 Gerrit-PatchSet: 3 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon
[kudu-CR] tool: better handling for positional arguments
Todd Lipcon has posted comments on this change. Change subject: tool: better handling for positional arguments .. Patch Set 1: (1 comment) http://gerrit.cloudera.org:8080/#/c/4013/1/src/kudu/tools/tool_action.h File src/kudu/tools/tool_action.h: PS1, Line 187: All remaining arguments on the command line will be joined (via single : // space delimiter) to construct the parameter value. > I had three options: yea, option #2 seems more natural to me (most libraries I've used present the leftover flags as 'argv') -- 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 Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: Yes
[kudu-CR] tool: better handling for positional arguments
Hello Alexey Serbin, Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/4013 to look at the new patch set (#2). Change subject: tool: better handling for positional arguments .. tool: better handling for positional arguments This patch modifies Action to describe all of its argument requirements, not just gflags. As a result, we can improve usability in two major ways: 1. The parser itself can process positional arguments, leaving less work for individual actions. 2. The action help strings can describe positional arguments. Change-Id: Ic6aa517ccf1915e4d106bcc5f80a3abfeae03271 --- M src/kudu/tools/tool_action.cc M src/kudu/tools/tool_action.h M src/kudu/tools/tool_action_fs.cc M src/kudu/tools/tool_action_tablet.cc M src/kudu/tools/tool_main.cc M src/kudu/util/subprocess.cc 6 files changed, 246 insertions(+), 132 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/13/4013/2 -- To view, visit http://gerrit.cloudera.org:8080/4013 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: Ic6aa517ccf1915e4d106bcc5f80a3abfeae03271 Gerrit-PatchSet: 2 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon
[kudu-CR] tool: better handling for positional arguments
Kudu Jenkins has posted comments on this change. Change subject: tool: better handling for positional arguments .. Patch Set 2: Build Started http://104.196.14.100/job/kudu-gerrit/2974/ -- 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: 2 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: No
[kudu-CR] tool: better handling for positional arguments
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 ", 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 > 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 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 at runtime. 2. Keep them separate (as a vector 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. 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
[kudu-CR] tool: better handling for positional arguments
Dan Burkert has posted comments on this change. Change subject: tool: better handling for positional arguments .. Patch Set 1: (2 comments) http://gerrit.cloudera.org:8080/#/c/4013/1/src/kudu/tools/tool_action.h File src/kudu/tools/tool_action.h: PS1, Line 135: marshaled I think it's clearer to just say 'put into the map'. PS1, Line 191: Required Why are variadic params required? -- 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 Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: Yes
[kudu-CR] tool: better handling for positional arguments
Todd Lipcon has posted comments on this change. Change subject: tool: better handling for positional arguments .. Patch Set 1: (4 comments) Sure wish there was some test coverage on these tools... http://gerrit.cloudera.org:8080/#/c/4013/1/src/kudu/tools/tool_action.h File src/kudu/tools/tool_action.h: PS1, Line 149: ActionArg the name sounds like it's a descriptor for one argument, whereas really it's a descriptor for all of the arguments. Rename to ArgsDescriptor? or ActionArgsInfo? Line 150: struct Arg { I think this being a nested struct is a bit confusing, because, best I can tell, 'ActionArgDescriptor' is an internal thing, and yet now you've got it exposed here looking like it's part of the API. 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 list of files, where the files may have spaces in their name... eg: kudu log-dump "/home/todd/My Stuff/log-1" "/home/todd/My Stuff/log-2" Also worth noting that this inherently verifies that at least one such argument is passed, right? 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? ie you should have used normal gflag macros to define it, and here you are just referring to it? What happens if you specify an undefined flag? Crash? Does the value of this flag end up in the 'args' map? Or just set into the gflag? -- 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 Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: Yes
[kudu-CR] tool: better handling for positional arguments
Alexey Serbin has posted comments on this change. Change subject: tool: better handling for positional arguments .. Patch Set 1: > (5 comments) > > 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. I re-read my comment and it seemed like I was too much advocating for using Golang :) In fact, I mostly meant it might be useful/insightful to take a look at the cli package. To me it looked very convenient the way how to specify commands, sub-commands and options in the 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. -- 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 Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: No
[kudu-CR] tool: better handling for positional arguments
Alexey Serbin has posted comments on this change. Change subject: tool: better handling for positional arguments .. Patch Set 1: Code-Review+1 (5 comments) 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. 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_idx variable? PS1, Line 164: int string::size_type ? http://gerrit.cloudera.org:8080/#/c/4013/1/src/kudu/tools/tool_action.h File src/kudu/tools/tool_action.h: Line 21: #include Nit: if I'm not mistaken, by the code style the boost and glog headers should come after STL headers and before kudu-specific ones. Line 162: std::vector optional; Does it make sense to have description for an optional parameter as well? As I see, both required and variadic ones have descriptions. 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 because operator-> is overloaded for the operand? I.e. why not const ActionArgDescriptor& args = action->args(); -- 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 Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: Yes
[kudu-CR] tool: better handling for positional arguments
Hello Dan Burkert, Todd Lipcon, I'd like you to do a code review. Please visit http://gerrit.cloudera.org:8080/4013 to review the following change. Change subject: tool: better handling for positional arguments .. tool: better handling for positional arguments This patch modifies Action to describe all of its argument requirements, not just gflags. As a result, we can improve usability in two major ways: 1. The parser itself can process positional arguments, leaving less work for individual actions. 2. The action help strings can describe positional arguments. Change-Id: Ic6aa517ccf1915e4d106bcc5f80a3abfeae03271 --- M src/kudu/tools/tool_action.cc M src/kudu/tools/tool_action.h M src/kudu/tools/tool_action_fs.cc M src/kudu/tools/tool_action_tablet.cc M src/kudu/tools/tool_main.cc M src/kudu/util/subprocess.cc 6 files changed, 232 insertions(+), 126 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/13/4013/1 -- To view, visit http://gerrit.cloudera.org:8080/4013 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newchange Gerrit-Change-Id: Ic6aa517ccf1915e4d106bcc5f80a3abfeae03271 Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Todd Lipcon
[kudu-CR] tool: better handling for positional arguments
Kudu Jenkins has posted comments on this change. Change subject: tool: better handling for positional arguments .. Patch Set 1: Build Started http://104.196.14.100/job/kudu-gerrit/2959/ -- 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 Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: No