Add beginnings of new cr command

Add beginnings of new cr command

This CL
* Establishes the root cr command, including subcommand
  dispatch and help
* Creates two minimal libraries for managing subcommands
  and terminal interaction
* Implements the help command and the beginning of the
  firstrun command

R=seanmccullough@chromium.org
BUG=605772
Committed: https://chromium.googlesource.com/infra/infra/+/d6d8a0f314f854a0b9e4b191af331a764563920d

[agable, 2016-04-28 22:50:10]

This CL replaces https://codereview.chromium.org/1905233002

It doesn't require third-party dependencies. I've learned a lot more since I
started that other one :)

It hasn't undergone golint/govet yet; I'm doing that now.

[agable, 2016-04-28 23:21:38]

PTAL at patchset 2; our toolchain seems happy with the style and documentation.

[seanmccullough1, 2016-04-29 00:54:35]

seanmccullough@google.com changed reviewers:
+ seanmccullough@google.com

[seanmccullough1, 2016-04-29 00:54:37]

https://codereview.chromium.org/1929153002/diff/1/go/src/infra/tools/cr/cmd/f...
File go/src/infra/tools/cr/cmd/firstrun/firstrun_nix.go (right):

https://codereview.chromium.org/1929153002/diff/1/go/src/infra/tools/cr/cmd/f...
go/src/infra/tools/cr/cmd/firstrun/firstrun_nix.go:21: //
firstrunCheckNotInstalled is a sanity check to make sure that the user really
FYI unexported vars/funcs etc need not be GoDoc'd

https://codereview.chromium.org/1929153002/diff/1/go/src/infra/tools/cr/cmd/f...
go/src/infra/tools/cr/cmd/firstrun/firstrun_nix.go:26: terminal.Debug("$PATH is
%v\n", pathenv)
why not log.Printf?

https://codereview.chromium.org/1929153002/diff/1/go/src/infra/tools/cr/cmd/f...
go/src/infra/tools/cr/cmd/firstrun/firstrun_nix.go:79: 
all of these noop funcs: implement, replace with a TODO comment or remove from
cl

https://codereview.chromium.org/1929153002/diff/1/go/src/infra/tools/cr/cmd/h...
File go/src/infra/tools/cr/cmd/help/help.go (right):

https://codereview.chromium.org/1929153002/diff/1/go/src/infra/tools/cr/cmd/h...
go/src/infra/tools/cr/cmd/help/help.go:21: var RootCmd = &cobra.Command{
where is cobra imported?

https://codereview.chromium.org/1929153002/diff/20001/go/src/infra/tools/cr/c...
File go/src/infra/tools/cr/cmd/firstrun/firstrun.go (right):

https://codereview.chromium.org/1929153002/diff/20001/go/src/infra/tools/cr/c...
go/src/infra/tools/cr/cmd/firstrun/firstrun.go:29: installing the cr tool.
would "install" or "init" be a more obvious name than "firstrun"?

https://codereview.chromium.org/1929153002/diff/20001/go/src/infra/tools/cr/c...
go/src/infra/tools/cr/cmd/firstrun/firstrun.go:39: firstrunCheckNotInstalled()
return firstrunCheckNotInstalled()?

https://codereview.chromium.org/1929153002/diff/20001/go/src/infra/tools/cr/c...
File go/src/infra/tools/cr/cmd/firstrun/firstrun_nix.go (right):

https://codereview.chromium.org/1929153002/diff/20001/go/src/infra/tools/cr/c...
go/src/infra/tools/cr/cmd/firstrun/firstrun_nix.go:1: // Copyright 2016 The
Chromium Authors. All rights reserved.
nit about the file name: standard libs tend to call the unix-specific files
foo_unix.go rather than foo_nix.go.

https://codereview.chromium.org/1929153002/diff/20001/go/src/infra/tools/cr/c...
go/src/infra/tools/cr/cmd/firstrun/firstrun_nix.go:43: if err != nil {
do something with err! Just return it if you don't have any way to make progress
in this func.

https://codereview.chromium.org/1929153002/diff/20001/go/src/infra/tools/cr/c...
File go/src/infra/tools/cr/cmd/firstrun/firstrun_windows.go (right):

https://codereview.chromium.org/1929153002/diff/20001/go/src/infra/tools/cr/c...
go/src/infra/tools/cr/cmd/firstrun/firstrun_windows.go:10: "bufio"
None of these packages are used in this file. I guess they might be used in
other files in this package and that lets it compile, but I'd still remove them
from this file.

https://codereview.chromium.org/1929153002/diff/20001/go/src/infra/tools/cr/h...
File go/src/infra/tools/cr/help.go (right):

https://codereview.chromium.org/1929153002/diff/20001/go/src/infra/tools/cr/h...
go/src/infra/tools/cr/help.go:32: helpcmd := subcommands[flags.Arg(0)]
You could also say 

if helpcmd, ok := subcommands[...]; ok {
   helpcmd.InitFlags(...)
   ...
} else {
  return fmt.Errorf(...)
}

It's a little more idiomatic but it does increase the indent level.

(And map element access is one place where exact return value count is not
enforced, so the 'ok' value is always optional.)

https://codereview.chromium.org/1929153002/diff/20001/go/src/infra/tools/cr/h...
go/src/infra/tools/cr/help.go:41: var helpCmd = subcommand.New(shortHelp,
longHelp, nil, helpRun)
The existence of helpCmd here and helpcmd (lower c) above is kind of confusing,
but I think I see what you're doing now. Maybe rename helpcmd above as
cmdForHelp or something else more distinct?

https://codereview.chromium.org/1929153002/diff/20001/go/src/infra/tools/cr/l...
File go/src/infra/tools/cr/lib/subcommand/subcommand.go (right):

https://codereview.chromium.org/1929153002/diff/20001/go/src/infra/tools/cr/l...
go/src/infra/tools/cr/lib/subcommand/subcommand.go:21: type Subcommand struct {
SubCmd would be a fine name too.

https://codereview.chromium.org/1929153002/diff/20001/go/src/infra/tools/cr/l...
go/src/infra/tools/cr/lib/subcommand/subcommand.go:22: ShortHelp string
godoc for ShortHelp or don't export it (doesn't appear to be a necessary export,
FWIW). Exported structs with no public fields (just public methods) are actually
pretty common.

https://codereview.chromium.org/1929153002/diff/20001/go/src/infra/tools/cr/l...
go/src/infra/tools/cr/lib/subcommand/subcommand.go:36: fmt.Println(c.ShortHelp)
fmt.Printf("%s\n\n%s\n\n", c.ShortHelp, c.LongHelp)

https://codereview.chromium.org/1929153002/diff/20001/go/src/infra/tools/cr/l...
File go/src/infra/tools/cr/lib/terminal/terminal.go (right):

https://codereview.chromium.org/1929153002/diff/20001/go/src/infra/tools/cr/l...
go/src/infra/tools/cr/lib/terminal/terminal.go:17: // SetDebug sets the internal
state to start/stop printing lines sent to Debug.
Just make var ShowDebug exported and get rid of this setter. Though, see comment
below about whether or not you really need this at all.

https://codereview.chromium.org/1929153002/diff/20001/go/src/infra/tools/cr/l...
go/src/infra/tools/cr/lib/terminal/terminal.go:23: func Print(format string,
args ...interface{}) {
Just use fmt.Printf (or log.Printf)?

https://codereview.chromium.org/1929153002/diff/20001/go/src/infra/tools/cr/l...
go/src/infra/tools/cr/lib/terminal/terminal.go:29: func Debug(format string,
args ...interface{}) {
I'd just use the standard logging packages for now, and make sure to remove
debug Printf statements from your CL before submitting.

https://codereview.chromium.org/1929153002/diff/20001/go/src/infra/tools/cr/m...
File go/src/infra/tools/cr/main.go (right):

https://codereview.chromium.org/1929153002/diff/20001/go/src/infra/tools/cr/m...
go/src/infra/tools/cr/main.go:39: colwriter := tabwriter.NewWriter(os.Stdout, 0,
0, 2, ' ', 0)
nice, I hadn't run across tabriter before.

https://codereview.chromium.org/1929153002/diff/20001/go/src/infra/tools/cr/m...
go/src/infra/tools/cr/main.go:48: fmt.Println("The cr tool intelligently manages
the Chrome Infra command-line SDK.")
fmt.Print(`

// all this stuff

`)

https://codereview.chromium.org/1929153002/diff/20001/go/src/infra/tools/cr/m...
go/src/infra/tools/cr/main.go:57: subcommands["help"] = helpCmd
https://golang.org/ref/spec#Package_initialization

Did you try using a package init() func? 

var (
  subcommands map[string]*subcommand.Subcommand
)

func init() {
  subcommands = make(map.....)
  subcommands["help"] = helpCmd
  subcommands["firstrun"] = firstrun.FirstrunCmd
}

====== or ======

have map[string]*subcommand.Subcommand exist in package subcommand. When you
call New(...) it automatically adds the subcmd to the map.

https://codereview.chromium.org/1929153002/diff/20001/go/src/infra/tools/cr/m...
go/src/infra/tools/cr/main.go:65: cmd = subcommands[os.Args[1]]
same comment as before with cmd, ok := ...

[agable, 2016-05-05 23:59:43]

Patchset responding to comments, but not finishing implementation of firstrun
command, now available.

https://codereview.chromium.org/1929153002/diff/1/go/src/infra/tools/cr/cmd/f...
File go/src/infra/tools/cr/cmd/firstrun/firstrun_nix.go (right):

https://codereview.chromium.org/1929153002/diff/1/go/src/infra/tools/cr/cmd/f...
go/src/infra/tools/cr/cmd/firstrun/firstrun_nix.go:79: 
On 2016/04/29 at 00:54:36, seanmccullough1 wrote:
> all of these noop funcs: implement, replace with a TODO comment or remove from
cl

Planning to implement. Coming soon.

https://codereview.chromium.org/1929153002/diff/1/go/src/infra/tools/cr/cmd/h...
File go/src/infra/tools/cr/cmd/help/help.go (right):

https://codereview.chromium.org/1929153002/diff/1/go/src/infra/tools/cr/cmd/h...
go/src/infra/tools/cr/cmd/help/help.go:21: var RootCmd = &cobra.Command{
On 2016/04/29 at 00:54:36, seanmccullough1 wrote:
> where is cobra imported?

This whole file is gone (old patchset).

https://codereview.chromium.org/1929153002/diff/20001/go/src/infra/tools/cr/c...
File go/src/infra/tools/cr/cmd/firstrun/firstrun.go (right):

https://codereview.chromium.org/1929153002/diff/20001/go/src/infra/tools/cr/c...
go/src/infra/tools/cr/cmd/firstrun/firstrun.go:29: installing the cr tool.
On 2016/04/29 at 00:54:36, seanmccullough1 wrote:
> would "install" or "init" be a more obvious name than "firstrun"?

Wanted to avoid "install", because that seems likely to be confused with
"install module X", which is very different functionality. Init is a good idea,
though.

Not sure how necessary "obvious" is though? The documents will say "download
this binary, then run ./cr firstrun". And I really wanted to avoid the command
name being something that people would say "oh I wonder if this will fix my
problem" later on down the line.

So I'm not gonna change the name for now, but will continue to consider it.

https://codereview.chromium.org/1929153002/diff/20001/go/src/infra/tools/cr/c...
go/src/infra/tools/cr/cmd/firstrun/firstrun.go:39: firstrunCheckNotInstalled()
On 2016/04/29 at 00:54:36, seanmccullough1 wrote:
> return firstrunCheckNotInstalled()?

Done.

https://codereview.chromium.org/1929153002/diff/20001/go/src/infra/tools/cr/c...
File go/src/infra/tools/cr/cmd/firstrun/firstrun_nix.go (right):

https://codereview.chromium.org/1929153002/diff/20001/go/src/infra/tools/cr/c...
go/src/infra/tools/cr/cmd/firstrun/firstrun_nix.go:1: // Copyright 2016 The
Chromium Authors. All rights reserved.
On 2016/04/29 at 00:54:36, seanmccullough1 wrote:
> nit about the file name: standard libs tend to call the unix-specific files
foo_unix.go rather than foo_nix.go.

Sweet, thanks!

https://codereview.chromium.org/1929153002/diff/20001/go/src/infra/tools/cr/c...
go/src/infra/tools/cr/cmd/firstrun/firstrun_nix.go:43: if err != nil {
On 2016/04/29 at 00:54:36, seanmccullough1 wrote:
> do something with err! Just return it if you don't have any way to make
progress in this func.

Like your comment about empty functions below, this is just a stub. Things will
be done with it in an upcoming patchset.

https://codereview.chromium.org/1929153002/diff/20001/go/src/infra/tools/cr/c...
File go/src/infra/tools/cr/cmd/firstrun/firstrun_windows.go (right):

https://codereview.chromium.org/1929153002/diff/20001/go/src/infra/tools/cr/c...
go/src/infra/tools/cr/cmd/firstrun/firstrun_windows.go:10: "bufio"
On 2016/04/29 at 00:54:36, seanmccullough1 wrote:
> None of these packages are used in this file. I guess they might be used in
other files in this package and that lets it compile, but I'd still remove them
from this file.

Right, removed.

https://codereview.chromium.org/1929153002/diff/20001/go/src/infra/tools/cr/h...
File go/src/infra/tools/cr/help.go (right):

https://codereview.chromium.org/1929153002/diff/20001/go/src/infra/tools/cr/h...
go/src/infra/tools/cr/help.go:32: helpcmd := subcommands[flags.Arg(0)]
On 2016/04/29 at 00:54:36, seanmccullough1 wrote:
> You could also say 
> 
> if helpcmd, ok := subcommands[...]; ok {
>    helpcmd.InitFlags(...)
>    ...
> } else {
>   return fmt.Errorf(...)
> }
> 
> It's a little more idiomatic but it does increase the indent level.
> 
> (And map element access is one place where exact return value count is not
enforced, so the 'ok' value is always optional.)

Shiny, didn't know about that. Done.

https://codereview.chromium.org/1929153002/diff/20001/go/src/infra/tools/cr/h...
go/src/infra/tools/cr/help.go:41: var helpCmd = subcommand.New(shortHelp,
longHelp, nil, helpRun)
On 2016/04/29 at 00:54:36, seanmccullough1 wrote:
> The existence of helpCmd here and helpcmd (lower c) above is kind of
confusing, but I think I see what you're doing now. Maybe rename helpcmd above
as cmdForHelp or something else more distinct?

Good idea, done.

https://codereview.chromium.org/1929153002/diff/20001/go/src/infra/tools/cr/l...
File go/src/infra/tools/cr/lib/subcommand/subcommand.go (right):

https://codereview.chromium.org/1929153002/diff/20001/go/src/infra/tools/cr/l...
go/src/infra/tools/cr/lib/subcommand/subcommand.go:22: ShortHelp string
On 2016/04/29 at 00:54:36, seanmccullough1 wrote:
> godoc for ShortHelp or don't export it (doesn't appear to be a necessary
export, FWIW). Exported structs with no public fields (just public methods) are
actually pretty common.

main.go directly access ShortHelp to print the list of all available
subcommands. But since I moved the list of Subcommands into the subcommand
package, I no longer need to export either of these.

https://codereview.chromium.org/1929153002/diff/20001/go/src/infra/tools/cr/l...
go/src/infra/tools/cr/lib/subcommand/subcommand.go:36: fmt.Println(c.ShortHelp)
On 2016/04/29 at 00:54:36, seanmccullough1 wrote:
> fmt.Printf("%s\n\n%s\n\n", c.ShortHelp, c.LongHelp)

Done.

https://codereview.chromium.org/1929153002/diff/20001/go/src/infra/tools/cr/l...
File go/src/infra/tools/cr/lib/terminal/terminal.go (right):

https://codereview.chromium.org/1929153002/diff/20001/go/src/infra/tools/cr/l...
go/src/infra/tools/cr/lib/terminal/terminal.go:17: // SetDebug sets the internal
state to start/stop printing lines sent to Debug.
On 2016/04/29 at 00:54:37, seanmccullough1 wrote:
> Just make var ShowDebug exported and get rid of this setter. Though, see
comment below about whether or not you really need this at all.

Done. See reply below for whether or not this package is necessary at all.

https://codereview.chromium.org/1929153002/diff/20001/go/src/infra/tools/cr/l...
go/src/infra/tools/cr/lib/terminal/terminal.go:29: func Debug(format string,
args ...interface{}) {
On 2016/04/29 at 00:54:37, seanmccullough1 wrote:
> I'd just use the standard logging packages for now, and make sure to remove
debug Printf statements from your CL before submitting.

Command line tools without verbosity levels make me sad. In the default case,
you want to print as little as possible to convince the user that useful work is
being done. In the failure case, you need to print checkpoints and interesting
information so that the user has a useful log they can provide to you. I felt it
was important to get some form of normal/verbose printing in here early on, so
that I could use it without having to think about it later.

These aren't for my personal debugging -- I use Printf for that and remove it
before sending the CL. These are for the actual end-user.

https://codereview.chromium.org/1929153002/diff/20001/go/src/infra/tools/cr/m...
File go/src/infra/tools/cr/main.go (right):

https://codereview.chromium.org/1929153002/diff/20001/go/src/infra/tools/cr/m...
go/src/infra/tools/cr/main.go:48: fmt.Println("The cr tool intelligently manages
the Chrome Infra command-line SDK.")
On 2016/04/29 at 00:54:37, seanmccullough1 wrote:
> fmt.Print(`
> 
> // all this stuff
> 
> `)

Done.

https://codereview.chromium.org/1929153002/diff/20001/go/src/infra/tools/cr/m...
go/src/infra/tools/cr/main.go:57: subcommands["help"] = helpCmd
On 2016/04/29 at 00:54:37, seanmccullough1 wrote:
> https://golang.org/ref/spec#Package_initialization
> 
> Did you try using a package init() func? 
> 
> var (
>   subcommands map[string]*subcommand.Subcommand
> )
> 
> func init() {
>   subcommands = make(map.....)
>   subcommands["help"] = helpCmd
>   subcommands["firstrun"] = firstrun.FirstrunCmd
> }
> 
> ====== or ======
> 
> have map[string]*subcommand.Subcommand exist in package subcommand. When you
call New(...) it automatically adds the subcmd to the map.

Oh good idea on the second suggestion, I like keeping the map in the subcommand
package. Done that.

https://codereview.chromium.org/1929153002/diff/20001/go/src/infra/tools/cr/m...
go/src/infra/tools/cr/main.go:65: cmd = subcommands[os.Args[1]]
On 2016/04/29 at 00:54:37, seanmccullough1 wrote:
> same comment as before with cmd, ok := ...

This puts the entire rest of main() inside the if-block, because cmd is defined
in the if statement and falls out of scope at the end. Is that really the best
way to do it? I'd rather have exceptional code be indented and normal code be
straight-line.

[seanmccullough1, 2016-05-06 00:43:54]

https://codereview.chromium.org/1929153002/diff/20001/go/src/infra/tools/cr/m...
File go/src/infra/tools/cr/main.go (right):

https://codereview.chromium.org/1929153002/diff/20001/go/src/infra/tools/cr/m...
go/src/infra/tools/cr/main.go:65: cmd = subcommands[os.Args[1]]
On 2016/05/05 23:59:43, agable wrote:
> On 2016/04/29 at 00:54:37, seanmccullough1 wrote:
> > same comment as before with cmd, ok := ...
> 
> This puts the entire rest of main() inside the if-block, because cmd is
defined
> in the if statement and falls out of scope at the end. Is that really the best
> way to do it? I'd rather have exceptional code be indented and normal code be
> straight-line.

sgtm

https://codereview.chromium.org/1929153002/diff/40001/go/src/infra/tools/cr/c...
File go/src/infra/tools/cr/cmd/firstrun/firstrun.go (right):

https://codereview.chromium.org/1929153002/diff/40001/go/src/infra/tools/cr/c...
go/src/infra/tools/cr/cmd/firstrun/firstrun.go:39: firstrunCheckNotInstalled()
What do you think of having stubbable funcs return errors (if only to indicate
they aren't implemented yet for a particular build)?

ex: https://golang.org/src/os/user/lookup_stubs.go

I don't know if _stubs.go is an established pattern, but returning errors at
least seems like a good idea in this situation.

https://codereview.chromium.org/1929153002/diff/40001/go/src/infra/tools/cr/c...
File go/src/infra/tools/cr/cmd/firstrun/firstrun_unix.go (right):

https://codereview.chromium.org/1929153002/diff/40001/go/src/infra/tools/cr/c...
go/src/infra/tools/cr/cmd/firstrun/firstrun_unix.go:36: // Check to see if an
executable called 'cr' is in that path.
If this comment (and many others here) is a TODO, please mark it TODO.

https://codereview.chromium.org/1929153002/diff/40001/go/src/infra/tools/cr/c...
go/src/infra/tools/cr/cmd/firstrun/firstrun_unix.go:43: if err != nil {
returning err here would fit naturally.

https://codereview.chromium.org/1929153002/diff/40001/go/src/infra/tools/cr/h...
File go/src/infra/tools/cr/help.go (right):

https://codereview.chromium.org/1929153002/diff/40001/go/src/infra/tools/cr/h...
go/src/infra/tools/cr/help.go:32: if cmdForHelp, ok :=
subcommand.Subcommands[flags.Arg(0)]; ok {
oh, I was thinking Subcommands would be subcommands, unexported in package
subcommand (and you'd access via Get() or some such. This is okay I guess, but
it is possible for code outside of package subcommand to potentially add/remove
elements from this map.

https://codereview.chromium.org/1929153002/diff/40001/go/src/infra/tools/cr/l...
File go/src/infra/tools/cr/lib/subcommand/subcommand.go (right):

https://codereview.chromium.org/1929153002/diff/40001/go/src/infra/tools/cr/l...
go/src/infra/tools/cr/lib/subcommand/subcommand.go:32: var Subcommands
map[string]*Subcommand
var Subcommands = make(map[string]*Subcommand)

and you can get rid of the init() below.

https://codereview.chromium.org/1929153002/diff/40001/go/src/infra/tools/cr/l...
go/src/infra/tools/cr/lib/subcommand/subcommand.go:35: // short help strings. If
no formatter is supplied, it uses a sane default.
Might want to mention that it flushes formatter before returning, or just not
make it a parameter until you actually need to (if ever).

https://codereview.chromium.org/1929153002/diff/40001/go/src/infra/tools/cr/l...
go/src/infra/tools/cr/lib/subcommand/subcommand.go:49: Subcommands[name] = ret
bonus points: check Subcommands for existence of name before adding it, and
return an error indicating a collision in that case.

[agable, 2016-05-10 00:34:11]

https://codereview.chromium.org/1929153002/diff/40001/go/src/infra/tools/cr/c...
File go/src/infra/tools/cr/cmd/firstrun/firstrun.go (right):

https://codereview.chromium.org/1929153002/diff/40001/go/src/infra/tools/cr/c...
go/src/infra/tools/cr/cmd/firstrun/firstrun.go:39: firstrunCheckNotInstalled()
On 2016/05/06 at 00:43:54, seanmccullough1 wrote:
> What do you think of having stubbable funcs return errors (if only to indicate
they aren't implemented yet for a particular build)?
> 
> ex: https://golang.org/src/os/user/lookup_stubs.go
> 
> I don't know if _stubs.go is an established pattern, but returning errors at
least seems like a good idea in this situation.

Yep, great idea. They now return errors.

https://codereview.chromium.org/1929153002/diff/40001/go/src/infra/tools/cr/c...
File go/src/infra/tools/cr/cmd/firstrun/firstrun_unix.go (right):

https://codereview.chromium.org/1929153002/diff/40001/go/src/infra/tools/cr/c...
go/src/infra/tools/cr/cmd/firstrun/firstrun_unix.go:36: // Check to see if an
executable called 'cr' is in that path.
On 2016/05/06 at 00:43:54, seanmccullough1 wrote:
> If this comment (and many others here) is a TODO, please mark it TODO.

Done.

https://codereview.chromium.org/1929153002/diff/40001/go/src/infra/tools/cr/c...
go/src/infra/tools/cr/cmd/firstrun/firstrun_unix.go:43: if err != nil {
On 2016/05/06 at 00:43:54, seanmccullough1 wrote:
> returning err here would fit naturally.

Removed the stub, returns an error now.

https://codereview.chromium.org/1929153002/diff/40001/go/src/infra/tools/cr/l...
File go/src/infra/tools/cr/lib/subcommand/subcommand.go (right):

https://codereview.chromium.org/1929153002/diff/40001/go/src/infra/tools/cr/l...
go/src/infra/tools/cr/lib/subcommand/subcommand.go:32: var Subcommands
map[string]*Subcommand
On 2016/05/06 at 00:43:54, seanmccullough1 wrote:
> var Subcommands = make(map[string]*Subcommand)
> 
> and you can get rid of the init() below.

...I could have sworn I tried exactly that, and only did the init as a last
resort. Guess I somehow missed that, or messed something else up at the same
time. Thanks!

https://codereview.chromium.org/1929153002/diff/40001/go/src/infra/tools/cr/l...
go/src/infra/tools/cr/lib/subcommand/subcommand.go:35: // short help strings. If
no formatter is supplied, it uses a sane default.
On 2016/05/06 at 00:43:54, seanmccullough1 wrote:
> Might want to mention that it flushes formatter before returning, or just not
make it a parameter until you actually need to (if ever).

Yeah, removed the parameter.

https://codereview.chromium.org/1929153002/diff/40001/go/src/infra/tools/cr/l...
go/src/infra/tools/cr/lib/subcommand/subcommand.go:49: Subcommands[name] = ret
On 2016/05/06 at 00:43:54, seanmccullough1 wrote:
> bonus points: check Subcommands for existence of name before adding it, and
return an error indicating a collision in that case.

Done

[seanmccullough1, 2016-05-10 02:00:57]

lgtm

[agable, 2016-05-10 17:44:50]

The CQ bit was checked by agable@chromium.org

[commit-bot: I haz the power, 2016-05-10 17:44:53]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1929153002/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1929153002/80001

[commit-bot: I haz the power, 2016-05-10 17:48:03]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-05-10 17:48:03]

Try jobs failed on following builders:
  Infra Linux Precise 32 Tester on tryserver.infra (JOB_FAILED,
https://build.chromium.org/p/tryserver.infra/builders/Infra%20Linux%20Precise...)
  Infra Mac Tester on tryserver.infra (JOB_FAILED,
https://build.chromium.org/p/tryserver.infra/builders/Infra%20Mac%20Tester/bu...)
  Infra Win Tester on tryserver.infra (JOB_FAILED,
https://build.chromium.org/p/tryserver.infra/builders/Infra%20Win%20Tester/bu...)

[agable, 2016-05-10 17:49:08]

We enforce coverage! Cool! Adding tests.

[seanmccullough1, 2016-06-03 16:47:29]

On 2016/05/10 17:49:08, agable wrote:
> We enforce coverage! Cool! Adding tests.

Any plans to submit this?

[agable, 2016-06-03 17:03:46]

On 2016/06/03 at 16:47:29, seanmccullough wrote:
> On 2016/05/10 17:49:08, agable wrote:
> > We enforce coverage! Cool! Adding tests.
> 
> Any plans to submit this?

Yep! I have a few more tests I want to add. Just got distracted with gerrit
migration and backlog and such.

[agable, 2016-06-03 19:55:45]

The CQ bit was checked by agable@chromium.org

[agable, 2016-06-03 19:55:46]

The patchset sent to the CQ was uploaded after l-g-t-m from
seanmccullough@google.com
Link to the patchset: https://codereview.chromium.org/1929153002/#ps120001
(title: "Fix lint")

[commit-bot: I haz the power, 2016-06-03 19:55:54]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1929153002/120001

[commit-bot: I haz the power, 2016-06-03 20:03:49]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-06-03 20:03:50]

Try jobs failed on following builders:
  Infra Linux Trusty 64 Tester on tryserver.infra (JOB_FAILED,
https://build.chromium.org/p/tryserver.infra/builders/Infra%20Linux%20Trusty%...)

[agable, 2016-06-03 20:48:25]

The CQ bit was checked by agable@chromium.org

[agable, 2016-06-03 20:48:26]

The patchset sent to the CQ was uploaded after l-g-t-m from
seanmccullough@google.com
Link to the patchset: https://codereview.chromium.org/1929153002/#ps140001
(title: "coverage expectations")

[commit-bot: I haz the power, 2016-06-03 20:48:29]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1929153002/140001

[commit-bot: I haz the power, 2016-06-03 20:56:45]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-06-03 20:56:47]

Try jobs failed on following builders:
  Infra Win Tester on tryserver.infra (JOB_FAILED,
https://build.chromium.org/p/tryserver.infra/builders/Infra%20Win%20Tester/bu...)

[agable, 2016-06-03 21:25:04]

The CQ bit was checked by agable@chromium.org

[agable, 2016-06-03 21:25:05]

The patchset sent to the CQ was uploaded after l-g-t-m from
seanmccullough@google.com
Link to the patchset: https://codereview.chromium.org/1929153002/#ps160001
(title: "Fix windows arguments")

[commit-bot: I haz the power, 2016-06-03 21:25:13]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1929153002/160001

[commit-bot: I haz the power, 2016-06-03 21:53:24]

Description was changed from

==========
Add beginnings of new cr command

This CL
* Establishes the root cr command, including subcommand
  dispatch and help
* Creates two minimal libraries for managing subcommands
  and terminal interaction
* Implements the help command and the beginning of the
  firstrun command

R=seanmccullough@chromium.org
BUG=605772
==========

to

==========
Add beginnings of new cr command

This CL
* Establishes the root cr command, including subcommand
  dispatch and help
* Creates two minimal libraries for managing subcommands
  and terminal interaction
* Implements the help command and the beginning of the
  firstrun command

R=seanmccullough@chromium.org
BUG=605772

Committed:
https://chromium.googlesource.com/infra/infra/+/d6d8a0f314f854a0b9e4b191af331...
==========

[commit-bot: I haz the power, 2016-06-03 21:53:26]

Committed patchset #9 (id:160001) as
https://chromium.googlesource.com/infra/infra/+/d6d8a0f314f854a0b9e4b191af331...