code review 161760043: cmd/digraph: digraph: a UNIX-style utility command for ...

cmd/digraph: digraph: a UNIX-style utility command for common operations on directed graphs in text format.

Example: show the transitive closure of imports of the digraph tool itself:

% go list -f '{{.ImportPath}}{{.Imports}}' ... | tr '[]' '  ' |
    digraph forward code.google.com/p/go.tools/cmd/digraph

+ basic test.

[adonovan, 2014-10-17 19:06:43]

Hello gri@golang.org (cc: golang-codereviews@googlegroups.com),

I'd like you to review this change to
https://code.google.com/p/go.tools

[gri, 2014-10-21 18:12:40]

Should also have tests.

https://codereview.appspot.com/161760043/diff/60001/cmd/digraph/digraph.go
File cmd/digraph/digraph.go (right):

https://codereview.appspot.com/161760043/diff/60001/cmd/digraph/digraph.go#ne...
cmd/digraph/digraph.go:34: The graph is provided on the standard input.
Add an example here. That would make this much clearer.

https://codereview.appspot.com/161760043/diff/60001/cmd/digraph/digraph.go#ne...
cmd/digraph/digraph.go:92: labels = append(labels, label)
use indexing if you allocated the space already

https://codereview.appspot.com/161760043/diff/60001/cmd/digraph/digraph.go#ne...
cmd/digraph/digraph.go:104: type graph map[string]nodeset
comment would be helpful

https://codereview.appspot.com/161760043/diff/60001/cmd/digraph/digraph.go#ne...
cmd/digraph/digraph.go:108: if !ok {
if edges == nil ?

and get rid of ok

https://codereview.appspot.com/161760043/diff/60001/cmd/digraph/digraph.go#ne...
cmd/digraph/digraph.go:199: fwd := make(graph)
fwd?

https://codereview.appspot.com/161760043/diff/60001/cmd/digraph/digraph.go#ne...
cmd/digraph/digraph.go:201: var linenum int
this is never used

https://codereview.appspot.com/161760043/diff/60001/cmd/digraph/digraph.go#ne...
cmd/digraph/digraph.go:217: if len(args) == 0 {
should do this in main() ?

https://codereview.appspot.com/161760043/diff/60001/cmd/digraph/digraph.go#ne...
cmd/digraph/digraph.go:223: fwd, err := parse(os.Stdin)
fwd?

why not g or graph or something like that?

even if they are forward edges it's a bit confusing a name

[adonovan, 2014-10-21 19:08:14]

On 2014/10/21 18:12:40, gri wrote:
> Should also have tests.
> 
> https://codereview.appspot.com/161760043/diff/60001/cmd/digraph/digraph.go
> File cmd/digraph/digraph.go (right):
> 
>
https://codereview.appspot.com/161760043/diff/60001/cmd/digraph/digraph.go#ne...
> cmd/digraph/digraph.go:34: The graph is provided on the standard input.
> Add an example here. That would make this much clearer.
> 
>
https://codereview.appspot.com/161760043/diff/60001/cmd/digraph/digraph.go#ne...
> cmd/digraph/digraph.go:92: labels = append(labels, label)
> use indexing if you allocated the space already
> 
>
https://codereview.appspot.com/161760043/diff/60001/cmd/digraph/digraph.go#ne...
> cmd/digraph/digraph.go:104: type graph map[string]nodeset
> comment would be helpful
> 
>
https://codereview.appspot.com/161760043/diff/60001/cmd/digraph/digraph.go#ne...
> cmd/digraph/digraph.go:108: if !ok {
> if edges == nil ?
> 
> and get rid of ok
> 
>
https://codereview.appspot.com/161760043/diff/60001/cmd/digraph/digraph.go#ne...
> cmd/digraph/digraph.go:199: fwd := make(graph)
> fwd?
> 
>
https://codereview.appspot.com/161760043/diff/60001/cmd/digraph/digraph.go#ne...
> cmd/digraph/digraph.go:201: var linenum int
> this is never used
> 
>
https://codereview.appspot.com/161760043/diff/60001/cmd/digraph/digraph.go#ne...
> cmd/digraph/digraph.go:217: if len(args) == 0 {
> should do this in main() ?
> 
>
https://codereview.appspot.com/161760043/diff/60001/cmd/digraph/digraph.go#ne...
> cmd/digraph/digraph.go:223: fwd, err := parse(os.Stdin)
> fwd?
> 
> why not g or graph or something like that?
> 
> even if they are forward edges it's a bit confusing a name

Thanks.  All done, added tests. PTAL.

[adonovan, 2014-10-21 19:08:20]

https://codereview.appspot.com/161760043/diff/60001/cmd/digraph/digraph.go
File cmd/digraph/digraph.go (right):

https://codereview.appspot.com/161760043/diff/60001/cmd/digraph/digraph.go#ne...
cmd/digraph/digraph.go:34: The graph is provided on the standard input.
On 2014/10/21 18:12:39, gri wrote:
> Add an example here. That would make this much clearer.

Done.

https://codereview.appspot.com/161760043/diff/60001/cmd/digraph/digraph.go#ne...
cmd/digraph/digraph.go:92: labels = append(labels, label)
On 2014/10/21 18:12:39, gri wrote:
> use indexing if you allocated the space already

Done.

(I was curious as to whether labels.len would be registerized so I benchmarked
it.  With gc, I found that the relative cost of a loop around append was 1 for
indexing, 1.25 for presized append, and 2.35 for resizing append.)

https://codereview.appspot.com/161760043/diff/60001/cmd/digraph/digraph.go#ne...
cmd/digraph/digraph.go:104: type graph map[string]nodeset
On 2014/10/21 18:12:39, gri wrote:
> comment would be helpful

Done.

https://codereview.appspot.com/161760043/diff/60001/cmd/digraph/digraph.go#ne...
cmd/digraph/digraph.go:108: if !ok {
On 2014/10/21 18:12:40, gri wrote:
> if edges == nil ?
> 
> and get rid of ok

Done.

https://codereview.appspot.com/161760043/diff/60001/cmd/digraph/digraph.go#ne...
cmd/digraph/digraph.go:199: fwd := make(graph)
On 2014/10/21 18:12:39, gri wrote:
> fwd?

Now 'g'.

https://codereview.appspot.com/161760043/diff/60001/cmd/digraph/digraph.go#ne...
cmd/digraph/digraph.go:201: var linenum int
On 2014/10/21 18:12:39, gri wrote:
> this is never used

A relic of a format in which not all byte streams were valid input. :)

https://codereview.appspot.com/161760043/diff/60001/cmd/digraph/digraph.go#ne...
cmd/digraph/digraph.go:217: if len(args) == 0 {
On 2014/10/21 18:12:39, gri wrote:
> should do this in main() ?

Done.

https://codereview.appspot.com/161760043/diff/60001/cmd/digraph/digraph.go#ne...
cmd/digraph/digraph.go:223: fwd, err := parse(os.Stdin)
On 2014/10/21 18:12:39, gri wrote:
> fwd?
> 
> why not g or graph or something like that?
> 
> even if they are forward edges it's a bit confusing a name

Ok.  'g' now.

[gri, 2014-10-21 19:32:36]

https://codereview.appspot.com/161760043/diff/80001/cmd/digraph/digraph.go
File cmd/digraph/digraph.go (right):

https://codereview.appspot.com/161760043/diff/80001/cmd/digraph/digraph.go#ne...
cmd/digraph/digraph.go:50: 
extra blank line

https://codereview.appspot.com/161760043/diff/80001/cmd/digraph/digraph.go#ne...
cmd/digraph/digraph.go:244: func digraph(args []string) error {
I'd pass cmd and args in here since you already do the check for len(args) > 0
outside. Now it's even less symmetric.

https://codereview.appspot.com/161760043/diff/80001/cmd/digraph/digraph.go#ne...
cmd/digraph/digraph.go:252: cmd := args[0]
move this into main()

https://codereview.appspot.com/161760043/diff/80001/cmd/digraph/digraph.go#ne...
cmd/digraph/digraph.go:409: return fmt.Errorf("no such query %q", cmd)
query -> command ?

https://codereview.appspot.com/161760043/diff/80001/cmd/digraph/digraph_test.go
File cmd/digraph/digraph_test.go (right):

https://codereview.appspot.com/161760043/diff/80001/cmd/digraph/digraph_test....
cmd/digraph/digraph_test.go:13: socks shoes
too bad this is not coming from the file clothes.txt

[adonovan, 2014-10-21 20:00:22]

https://codereview.appspot.com/161760043/diff/80001/cmd/digraph/digraph.go
File cmd/digraph/digraph.go (right):

https://codereview.appspot.com/161760043/diff/80001/cmd/digraph/digraph.go#ne...
cmd/digraph/digraph.go:50: 
On 2014/10/21 19:32:35, gri wrote:
> extra blank line

Done.

https://codereview.appspot.com/161760043/diff/80001/cmd/digraph/digraph.go#ne...
cmd/digraph/digraph.go:244: func digraph(args []string) error {
On 2014/10/21 19:32:35, gri wrote:
> I'd pass cmd and args in here since you already do the check for len(args) > 0
> outside. Now it's even less symmetric.

Done.

https://codereview.appspot.com/161760043/diff/80001/cmd/digraph/digraph.go#ne...
cmd/digraph/digraph.go:252: cmd := args[0]
On 2014/10/21 19:32:35, gri wrote:
> move this into main()

Done.

https://codereview.appspot.com/161760043/diff/80001/cmd/digraph/digraph.go#ne...
cmd/digraph/digraph.go:409: return fmt.Errorf("no such query %q", cmd)
On 2014/10/21 19:32:35, gri wrote:
> query -> command ?

Done.

https://codereview.appspot.com/161760043/diff/80001/cmd/digraph/digraph_test.go
File cmd/digraph/digraph_test.go (right):

https://codereview.appspot.com/161760043/diff/80001/cmd/digraph/digraph_test....
cmd/digraph/digraph_test.go:13: socks shoes
On 2014/10/21 19:32:35, gri wrote:
> too bad this is not coming from the file clothes.txt

Do you mean you wish these string constants were combined?

[gri, 2014-10-21 20:02:37]

LGTM

https://codereview.appspot.com/161760043/diff/80001/cmd/digraph/digraph_test.go
File cmd/digraph/digraph_test.go (right):

https://codereview.appspot.com/161760043/diff/80001/cmd/digraph/digraph_test....
cmd/digraph/digraph_test.go:13: socks shoes
On 2014/10/21 20:00:21, adonovan wrote:
> On 2014/10/21 19:32:35, gri wrote:
> > too bad this is not coming from the file clothes.txt
> 
> Do you mean you wish these string constants were combined?

No, I meant that since you're referring to clothes.txt in your example, it would
be nice if there were actually a clothes.txt present, and this is the one you
use for the testing input.

[adonovan, 2014-10-21 20:20:30]

Thanks for the review.

https://codereview.appspot.com/161760043/diff/80001/cmd/digraph/digraph_test.go
File cmd/digraph/digraph_test.go (right):

https://codereview.appspot.com/161760043/diff/80001/cmd/digraph/digraph_test....
cmd/digraph/digraph_test.go:13: socks shoes
On 2014/10/21 20:02:37, gri wrote:
> On 2014/10/21 20:00:21, adonovan wrote:
> > On 2014/10/21 19:32:35, gri wrote:
> > > too bad this is not coming from the file clothes.txt
> > 
> > Do you mean you wish these string constants were combined?
> 
> No, I meant that since you're referring to clothes.txt in your example, it
would
> be nice if there were actually a clothes.txt present, and this is the one you
> use for the testing input.

I see.  I wouldn't expect the files mentioned in the -help output of a tool to
be part of its distribution though, so I'll leave it as is.

[adonovan, 2014-10-23 13:13:43]

*** Submitted as
https://code.google.com/p/go/source/detail?r=26bf67f2818a&repo=tools ***

cmd/digraph: digraph: a UNIX-style utility command for common operations on
directed graphs in text format.

Example: show the transitive closure of imports of the digraph tool itself:

% go list -f '{{.ImportPath}}{{.Imports}}' ... | tr '[]' '  ' |
    digraph forward code.google.com/p/go.tools/cmd/digraph

+ basic test.

LGTM=gri
R=gri, sameer
CC=golang-codereviews
https://codereview.appspot.com/161760043