Mojom formatter incorporated in the mojom binary.

Mojom formatter incorporated in the mojom binary.

BUG= #596
R=rudominer@chromium.org

Committed: https://chromium.googlesource.com/external/mojo/+/f6a8263d50bdc355aab5de741554077bb6bf15f9

[azani, 2016-02-19 00:25:40]

azani@chromium.org changed reviewers:
+ rudominer@chromium.org

[azani, 2016-02-19 00:25:40]

ptal

[rudominer, 2016-02-19 05:49:16]

https://codereview.chromium.org/1702873002/diff/140001/mojom/mojom_parser/moj...
File mojom/mojom_parser/mojom_main.go (right):

https://codereview.chromium.org/1702873002/diff/140001/mojom/mojom_parser/moj...
mojom/mojom_parser/mojom_main.go:17: //		mojom command [arguments]
put "command" and "arguments" in angle brackets to indicate that they are not
literals the way that "mojom" is.

 mojom <command> [<arguments>]

https://codereview.chromium.org/1702873002/diff/140001/mojom/mojom_parser/moj...
mojom/mojom_parser/mojom_main.go:20: // fmt_cmd.go.
Instead of this statement I would prefer a continuation of the description of
how the tool is invoked. So something like:

The tool is invoked as follows:

         mojom <command> [<arguments>]

where <command> is one of: 
 - parse
 - fmt

For for further information about each command see the file named 
<command>_command.go,  for example "parse_command.go" and "fmt_command.go".

https://codereview.chromium.org/1702873002/diff/140001/mojom/mojom_parser/moj...
mojom/mojom_parser/mojom_main.go:60: usage := "Mojom is a tool for managing
.mojom files.\n\n"
Instead of "Mojom" you should toolName here.

https://codereview.chromium.org/1702873002/diff/140001/mojom/mojom_parser/moj...
mojom/mojom_parser/mojom_main.go:62: usage += fmt.Sprintf("\t%s command
[arguments]\n\n", toolName)
Put angle brackets around "command" (and maybe also around "arguments"?)

https://codereview.chromium.org/1702873002/diff/140001/mojom/mojom_parser/moj...
mojom/mojom_parser/mojom_main.go:66: usage += fmt.Sprintf("\t%s\t%s\n", name,
c.commandMap[name].Desc)
use a byte.buffer to build up the usage string. see for example what you did in
printer.go.

https://codereview.chromium.org/1702873002/diff/140001/mojom/mojom_parser/par...
File mojom/mojom_parser/parse_cmd.go (right):

https://codereview.chromium.org/1702873002/diff/140001/mojom/mojom_parser/par...
mojom/mojom_parser/parse_cmd.go:71: flagSet := flag.NewFlagSet("parse_cmd",
flag.ContinueOnError)
The string "parse_cmd" will show up in user-facing messages which is not good.
You should probably use "parse".

https://codereview.chromium.org/1702873002/diff/140001/mojom/mojom_parser/par...
mojom/mojom_parser/parse_cmd.go:87: printUsage()
I patched in your cl and played around with this.
When I typed in "./mojom_parser_linux_amd64 parse -x"
I saw the usage printed twice, once automatically by the flag infrastructure and
once by this call:

rudominer@rudominer2:~/mojo/src$ ./mojom_parser_linux_amd64 parse -x
flag provided but not defined: -x
Usage of parse_cmd:
  -I value
    	comma-separated list of directory paths to search for mojom imports
(default [])
  -debug
    	Generate debug data including the parse tree and print it to standard out.
  -meta-data-only
    	Only parse file attributes and 'module' statement, not mojom declarations
or import statements.
  -out string
    	The path to the output file. If not present the output will be written to
standard out.
flag provided but not defined: -x
Usage: mojom_parser_linux_amd64 parse [-I <include_dirs>] [-out <out_file>]
[-debug] [-meta-data-only] <mojom_file>...

  -I value
    	comma-separated list of directory paths to search for mojom imports
(default [])
  -debug
    	Generate debug data including the parse tree and print it to standard out.
  -meta-data-only
    	Only parse file attributes and 'module' statement, not mojom declarations
or import statements.
  -out string
    	The path to the output file. If not present the output will be written to
standard out.

https://codereview.chromium.org/1702873002/diff/140001/mojom/mojom_parser/par...
File mojom/mojom_parser/parser_main.go (left):

https://codereview.chromium.org/1702873002/diff/140001/mojom/mojom_parser/par...
mojom/mojom_parser/parser_main.go:5: /*
I wonder why the code review tool shows this file as modified instead of
deleted?

[azani, 2016-02-19 19:28:30]

ptal

https://codereview.chromium.org/1702873002/diff/140001/mojom/mojom_parser/moj...
File mojom/mojom_parser/mojom_main.go (right):

https://codereview.chromium.org/1702873002/diff/140001/mojom/mojom_parser/moj...
mojom/mojom_parser/mojom_main.go:17: //		mojom command [arguments]
On 2016/02/19 05:49:16, rudominer wrote:
> put "command" and "arguments" in angle brackets to indicate that they are not
> literals the way that "mojom" is.
> 
>  mojom <command> [<arguments>]

Done.

https://codereview.chromium.org/1702873002/diff/140001/mojom/mojom_parser/moj...
mojom/mojom_parser/mojom_main.go:20: // fmt_cmd.go.
On 2016/02/19 05:49:16, rudominer wrote:
> Instead of this statement I would prefer a continuation of the description of
> how the tool is invoked. So something like:
> 
> The tool is invoked as follows:
> 
>          mojom <command> [<arguments>]
> 
> where <command> is one of: 
>  - parse
>  - fmt
> 
> For for further information about each command see the file named 
> <command>_command.go,  for example "parse_command.go" and "fmt_command.go".

Done.

https://codereview.chromium.org/1702873002/diff/140001/mojom/mojom_parser/moj...
mojom/mojom_parser/mojom_main.go:60: usage := "Mojom is a tool for managing
.mojom files.\n\n"
On 2016/02/19 05:49:16, rudominer wrote:
> Instead of "Mojom" you should toolName here.

Done.

https://codereview.chromium.org/1702873002/diff/140001/mojom/mojom_parser/moj...
mojom/mojom_parser/mojom_main.go:62: usage += fmt.Sprintf("\t%s command
[arguments]\n\n", toolName)
On 2016/02/19 05:49:16, rudominer wrote:
> Put angle brackets around "command" (and maybe also around "arguments"?)

Done.

https://codereview.chromium.org/1702873002/diff/140001/mojom/mojom_parser/moj...
mojom/mojom_parser/mojom_main.go:66: usage += fmt.Sprintf("\t%s\t%s\n", name,
c.commandMap[name].Desc)
On 2016/02/19 05:49:16, rudominer wrote:
> use a byte.buffer to build up the usage string. see for example what you did
in
> printer.go.

Done.

https://codereview.chromium.org/1702873002/diff/140001/mojom/mojom_parser/par...
File mojom/mojom_parser/parse_cmd.go (right):

https://codereview.chromium.org/1702873002/diff/140001/mojom/mojom_parser/par...
mojom/mojom_parser/parse_cmd.go:71: flagSet := flag.NewFlagSet("parse_cmd",
flag.ContinueOnError)
On 2016/02/19 05:49:16, rudominer wrote:
> The string "parse_cmd" will show up in user-facing messages which is not good.
> You should probably use "parse".

Done.

https://codereview.chromium.org/1702873002/diff/140001/mojom/mojom_parser/par...
mojom/mojom_parser/parse_cmd.go:87: printUsage()
On 2016/02/19 05:49:16, rudominer wrote:
> I patched in your cl and played around with this.
> When I typed in "./mojom_parser_linux_amd64 parse -x"
> I saw the usage printed twice, once automatically by the flag infrastructure
and
> once by this call:
> 
> rudominer@rudominer2:~/mojo/src$ ./mojom_parser_linux_amd64 parse -x
> flag provided but not defined: -x
> Usage of parse_cmd:
>   -I value
>     	comma-separated list of directory paths to search for mojom imports
> (default [])
>   -debug
>     	Generate debug data including the parse tree and print it to standard
out.
>   -meta-data-only
>     	Only parse file attributes and 'module' statement, not mojom declarations
> or import statements.
>   -out string
>     	The path to the output file. If not present the output will be written to
> standard out.
> flag provided but not defined: -x
> Usage: mojom_parser_linux_amd64 parse [-I <include_dirs>] [-out <out_file>]
> [-debug] [-meta-data-only] <mojom_file>...
> 
>   -I value
>     	comma-separated list of directory paths to search for mojom imports
> (default [])
>   -debug
>     	Generate debug data including the parse tree and print it to standard
out.
>   -meta-data-only
>     	Only parse file attributes and 'module' statement, not mojom declarations
> or import statements.
>   -out string
>     	The path to the output file. If not present the output will be written to
> standard out.

Done.

[rudominer, 2016-02-19 20:42:43]

https://codereview.chromium.org/1702873002/diff/180001/mojom/mojom_parser/moj...
File mojom/mojom_parser/mojom_main.go (right):

https://codereview.chromium.org/1702873002/diff/180001/mojom/mojom_parser/moj...
mojom/mojom_parser/mojom_main.go:93: fmt.Printf("%s is not a recognized
command.\n")
You forgot to put the name of the command into the Printf statement.

https://codereview.chromium.org/1702873002/diff/180001/mojom/mojom_parser/par...
File mojom/mojom_parser/parse_cmd.go (right):

https://codereview.chromium.org/1702873002/diff/180001/mojom/mojom_parser/par...
mojom/mojom_parser/parse_cmd.go:86: fmt.Fprintln(os.Stderr, err.Error())
This is still not working perfectly. When I type
./mojom_parser_linux_amd64 parse

I get the nice usage line, but when I type

./mojom_parser_linux_amd64 parse -h

I don't. I think we always want the nice usage line.

I'm reading my previous comment to you that caused you to remove the call to
printUsage() and I can't understand how the result that I claimed I was seeing
would be possible with the previous version of the code. But I copied and pasted
it from the console.
Would you please just play around with different possible user inputs and make
sure that the output message is good in all cases.
I think what we want is the usage line followed by the table of flag
definitions.

[azani, 2016-02-19 23:23:25]

ptal

https://codereview.chromium.org/1702873002/diff/180001/mojom/mojom_parser/moj...
File mojom/mojom_parser/mojom_main.go (right):

https://codereview.chromium.org/1702873002/diff/180001/mojom/mojom_parser/moj...
mojom/mojom_parser/mojom_main.go:93: fmt.Printf("%s is not a recognized
command.\n")
On 2016/02/19 20:42:42, rudominer wrote:
> You forgot to put the name of the command into the Printf statement.

Done.

https://codereview.chromium.org/1702873002/diff/180001/mojom/mojom_parser/par...
File mojom/mojom_parser/parse_cmd.go (right):

https://codereview.chromium.org/1702873002/diff/180001/mojom/mojom_parser/par...
mojom/mojom_parser/parse_cmd.go:86: fmt.Fprintln(os.Stderr, err.Error())
On 2016/02/19 20:42:42, rudominer wrote:
> This is still not working perfectly. When I type
> ./mojom_parser_linux_amd64 parse
> 
> I get the nice usage line, but when I type
> 
> ./mojom_parser_linux_amd64 parse -h
> 
> I don't. I think we always want the nice usage line.
> 
> I'm reading my previous comment to you that caused you to remove the call to
> printUsage() and I can't understand how the result that I claimed I was seeing
> would be possible with the previous version of the code. But I copied and
pasted
> it from the console.
> Would you please just play around with different possible user inputs and make
> sure that the output message is good in all cases.
> I think what we want is the usage line followed by the table of flag
> definitions.

Done.

[rudominer, 2016-02-20 05:53:49]

lgtm with some comment nits.

Thank you for perfecting this.

https://codereview.chromium.org/1702873002/diff/240001/mojom/mojom_parser/moj...
File mojom/mojom_parser/mojom_main.go (right):

https://codereview.chromium.org/1702873002/diff/240001/mojom/mojom_parser/moj...
mojom/mojom_parser/mojom_main.go:103: func GetFlagDefaults(f *flag.FlagSet)
string {
Seems like a strange name for this function. How about UsageString() or
HelpString()?

https://codereview.chromium.org/1702873002/diff/240001/mojom/mojom_parser/moj...
mojom/mojom_parser/mojom_main.go:106: // isZeroValue guesses if the string
represents the zero value for a flag.
explain why--how is this used?

https://codereview.chromium.org/1702873002/diff/240001/mojom/mojom_parser/moj...
mojom/mojom_parser/mojom_main.go:119: f.VisitAll(func(f *flag.Flag) {
Please add comment explaining what is happening here.

[azani, 2016-02-22 17:37:39]

https://codereview.chromium.org/1702873002/diff/240001/mojom/mojom_parser/moj...
File mojom/mojom_parser/mojom_main.go (right):

https://codereview.chromium.org/1702873002/diff/240001/mojom/mojom_parser/moj...
mojom/mojom_parser/mojom_main.go:103: func GetFlagDefaults(f *flag.FlagSet)
string {
On 2016/02/20 05:53:49, rudominer wrote:
> Seems like a strange name for this function. How about UsageString() or
> HelpString()?

Done.

https://codereview.chromium.org/1702873002/diff/240001/mojom/mojom_parser/moj...
mojom/mojom_parser/mojom_main.go:106: // isZeroValue guesses if the string
represents the zero value for a flag.
On 2016/02/20 05:53:49, rudominer wrote:
> explain why--how is this used?

Done.

https://codereview.chromium.org/1702873002/diff/240001/mojom/mojom_parser/moj...
mojom/mojom_parser/mojom_main.go:119: f.VisitAll(func(f *flag.Flag) {
On 2016/02/20 05:53:49, rudominer wrote:
> Please add comment explaining what is happening here.

Done.

[azani, 2016-02-22 17:37:56]

Description was changed from

==========
Mojom formatter incorporated in the mojom binary.

BUG= #596
==========

to

==========
Mojom formatter incorporated in the mojom binary.

BUG= #596
R=rudominer@chromium.org

Committed:
https://chromium.googlesource.com/external/mojo/+/f6a8263d50bdc355aab5de74155...
==========

[azani, 2016-02-22 17:37:58]

Committed patchset #14 (id:260001) manually as
f6a8263d50bdc355aab5de741554077bb6bf15f9 (presubmit successful).