code review 13532052: go.tools/cover: clarify mode is only necessary wh...

go.tools/cover: clean up usage information

[nathany, 2013-09-22 17:28:34]

Hello golang-dev@googlegroups.com,

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

[minux1, 2013-09-22 17:55:33]

R=r

[r, 2013-09-23 00:08:18]

https://codereview.appspot.com/13532052/diff/6001/cmd/cover/cover.go
File cmd/cover/cover.go (right):

https://codereview.appspot.com/13532052/diff/6001/cmd/cover/cover.go#newcode54
cmd/cover/cover.go:54: mode    = flag.String("mode", "", "coverage mode when
generating source: set, count, atomic")
it doesn't generate source, exactly. the help message says 'generating modified
source', which is closer but too long for this.

do you really find this confusing? i think the usageMessage is pretty clear.

[nathany, 2013-09-23 04:41:12]

On 2013/09/23 00:08:18, r wrote:
> https://codereview.appspot.com/13532052/diff/6001/cmd/cover/cover.go
> File cmd/cover/cover.go (right):
> 
> https://codereview.appspot.com/13532052/diff/6001/cmd/cover/cover.go#newcode54
> cmd/cover/cover.go:54: mode    = flag.String("mode", "", "coverage mode when
> generating source: set, count, atomic")
> it doesn't generate source, exactly. the help message says 'generating
modified
> source', which is closer but too long for this.
> 
> do you really find this confusing? i think the usageMessage is pretty clear.

Now that I know how it works, I don't find it so confusing, but I was puzzled
because I was scanning the help for what I needed, rather than reading every
word from top to bottom. That is pretty common, right?

While the first example "go tool cover -html=c.out" is exactly what I needed, I
had it in my head that it was only for the default "set" mode.

I read the examples until I saw "Generate modified source" which I wasn't
interested in, so I jumped down to flags. It was a few minutes before I noticed
the line "Only one of -html, -func, or -mode may be set" which set me on the
right track. This patch clarifies that mode is only to be used for generating
source, right at the point where someone is looking at the mode flag (rather
than five lines up).

If I were writing the documentation for this tool (or say, a blog post), I would
group the relevant flags with each scenario. Having all the Flags listed
together at the bottom is just a side effect of how the flag package works.

The other issue is that running an invalid command, such as:
go tool cover -html=c.out -mode="set"
Just spits out the usage information. It doesn't yell at me what I did wrong. Or
even that I did something wrong. For all I know, it could be a bug in release
candidate software.

Another way to avoid my initial confusion would be to update the 2nd line with
'go test -coverprofile=c.out -covermode=set'. This would provide a little more
information on the other tool, including to look up covermode (or just guess
based on mode below), and might have set in my head that "go tool cover
-html=c.out" was enough.

[r, 2013-09-23 04:44:04]

i like the suggestion in your final paragraph.

[nathany, 2013-09-23 04:52:13]

On 2013/09/23 04:44:04, r wrote:
> i like the suggestion in your final paragraph.

Thanks. I think talking it through helped. I'll go ahead and update the patch.

Shall I revert the previous change or leave it in?

[r, 2013-09-23 04:53:54]

take it out please.

[minux1, 2013-09-23 04:56:27]

also please remove the tab before the first line of the description.

[nathany, 2013-09-23 05:10:25]

On 2013/09/23 04:56:27, minux wrote:
> also please remove the tab before the first line of the description.

minux: I'm not sure I'm following. In the text or source? line?

May I take some creative license to avoid unsightly wrapping at 80-characters?

Usage of 'go tool cover', given a coverage profile produced by:
	go test -coverprofile=c.out -covermode=set

Open a web browser displaying annotated source code:
	go tool cover -html=c.out

...

[nathany, 2013-09-23 05:14:00]

Also, will Rietveld squash all my messiness into one nice commit before it gets
merged? Just `hg upload 13532052` and that's it?

[minux1, 2013-09-23 05:15:24]

On 2013/09/23 05:10:25, nathany wrote:
> On 2013/09/23 04:56:27, minux wrote:
> > also please remove the tab before the first line of the description.
> 
> minux: I'm not sure I'm following. In the text or source? line?
i'm not sure if it's caused by leading spaces or tab, but on the web page
https://codereview.appspot.com/13532052/, there is some spaces before
the first line of the issue description ("go.tools/cover: clarify mode is only
necessary when generating source.").

please see if you can remove them (click "edit issue" on the web page).

[minux1, 2013-09-23 05:18:40]

On Mon, Sep 23, 2013 at 1:14 AM, <nj@nathany.com> wrote:

> Also, will Rietveld squash all my messiness into one nice commit before
> it gets merged? Just `hg upload 13532052` and that's it?
>
yes. hg clpatch will only download and apply the latest patch form the CL,
so just "hg upl 13532052" will do.

[nathany, 2013-09-23 05:37:16]

On 2013/09/23 05:18:40, minux wrote:
> On Mon, Sep 23, 2013 at 1:14 AM, <mailto:nj@nathany.com> wrote:
> 
> > Also, will Rietveld squash all my messiness into one nice commit before
> > it gets merged? Just `hg upload 13532052` and that's it?
> >
> yes. hg clpatch will only download and apply the latest patch form the CL,
> so just "hg upl 13532052" will do.

Sorry for my Mercurial + Rietveld newbness, but hg upl 13532052 just says:
https://codereview.appspot.com/13532052

With the M modified file still there. Of course hg commit is disabled. I don't
do another hg change, do I?

[minux1, 2013-09-23 05:42:20]

On Mon, Sep 23, 2013 at 1:37 AM, <nj@nathany.com> wrote:

> On 2013/09/23 05:18:40, minux wrote:
>
>  On Mon, Sep 23, 2013 at 1:14 AM, <mailto:nj@nathany.com> wrote:
>>
>
>  > Also, will Rietveld squash all my messiness into one nice commit
>>
> before
>
>> > it gets merged? Just `hg upload 13532052` and that's it?
>> >
>> yes. hg clpatch will only download and apply the latest patch form the
>>
> CL,
>
>> so just "hg upl 13532052" will do.
>>
>
> Sorry for my Mercurial + Rietveld newbness, but hg upl 13532052 just
> says:
>
https://codereview.appspot.**com/13532052<https://codereview.appspot.com/1353...

this is correct output.

>
> With the M modified file still there. Of course hg commit is disabled. I
> don't do another hg change, do I?
>
if you open that link you will see there is a new patchset created with the
new
revision of code in your local tree. no, you don't need to do another hg
change,
and you've done it right. :-)

[nathany, 2013-09-23 05:45:34]

On 2013/09/23 05:42:20, minux wrote:
> On Mon, Sep 23, 2013 at 1:37 AM, <mailto:nj@nathany.com> wrote:
> 
> > On 2013/09/23 05:18:40, minux wrote:
> >
> >  On Mon, Sep 23, 2013 at 1:14 AM, <mailto:nj@nathany.com> wrote:
> >>
> >
> >  > Also, will Rietveld squash all my messiness into one nice commit
> >>
> > before
> >
> >> > it gets merged? Just `hg upload 13532052` and that's it?
> >> >
> >> yes. hg clpatch will only download and apply the latest patch form the
> >>
> > CL,
> >
> >> so just "hg upl 13532052" will do.
> >>
> >
> > Sorry for my Mercurial + Rietveld newbness, but hg upl 13532052 just
> > says:
> >
>
https://codereview.appspot.**com/13532052%3Chttps://codereview.appspot.com/13...>
> 
> this is correct output.
> 
> >
> > With the M modified file still there. Of course hg commit is disabled. I
> > don't do another hg change, do I?
> >
> if you open that link you will see there is a new patchset created with the
> new
> revision of code in your local tree. no, you don't need to do another hg
> change,
> and you've done it right. :-)

That's a nice surprise. :-)

So just ignore hg status and everything will be fine.

Hopefully that patch is satisfactory. It's good night for me.

[r, 2013-09-23 22:25:57]

https://codereview.appspot.com/13532052/diff/22001/cmd/cover/cover.go
File cmd/cover/cover.go (right):

https://codereview.appspot.com/13532052/diff/22001/cmd/cover/cover.go#newcode36
cmd/cover/cover.go:36: `Usage of 'go tool cover', given a coverage profile
produced by:
please undo this change.

https://codereview.appspot.com/13532052/diff/22001/cmd/cover/cover.go#newcode37
cmd/cover/cover.go:37: go test -coverprofile=c.out -covermode=set
now i think this just makes it seem like you have to specify the covermode when
you run go test for coverage. how about

Usage of 'go tool cover':

Given a coverage profile generated by
  go test -coverprofile=c.out
or
  go test -coverprofile=c.out -covermode=set
the following examples present the data in various forms:

https://codereview.appspot.com/13532052/diff/22001/cmd/cover/cover.go#newcode45
cmd/cover/cover.go:45: Generate modified source code with coverage annotations
(what go test -cover does):
insert a blank line, then

Finally, to generate....

https://codereview.appspot.com/13532052/diff/22001/cmd/cover/cover.go#newcode122
cmd/cover/cover.go:122: flag.Usage()
we need to fix this too. it should dump a huge message when you make a typo,
only when you explicitly ask for help.

go tool cover -coverprofil=c.out

should say something like

flag provided but not defined: -coverprofil
For full usage information, run "go tool cover -help"

and then

go tool cover -help

should dump the big scroll

[r, 2013-09-23 22:26:44]

p.s. we should make the -help change (printing help only when asked) separately.
i can do that if you like, or you can take a crack at it in a separate CL.

[nathany, 2013-09-24 02:27:43]

On 2013/09/23 22:26:44, r wrote:
> p.s. we should make the -help change (printing help only when asked)
separately.
> i can do that if you like, or you can take a crack at it in a separate CL.

I would be happy to take it on once this CL is ready to merge.

[nathany, 2013-09-24 02:29:27]

https://codereview.appspot.com/13532052/diff/22001/cmd/cover/cover.go
File cmd/cover/cover.go (right):

https://codereview.appspot.com/13532052/diff/22001/cmd/cover/cover.go#newcode37
cmd/cover/cover.go:37: go test -coverprofile=c.out -covermode=set
On 2013/09/23 22:25:58, r wrote:
> now i think this just makes it seem like you have to specify the covermode
when

Maybe adding -covermode=set wasn't a stroke of genius after all. This still
comes across as the only option instead of an example (other covermodes, etc.).

[nathany, 2013-09-24 02:44:31]

https://codereview.appspot.com/13532052/diff/31001/cmd/cover/cover.go
File cmd/cover/cover.go (right):

https://codereview.appspot.com/13532052/diff/31001/cmd/cover/cover.go#newcode48
cmd/cover/cover.go:48: go tool cover -func=c.out
This doesn't provide an example or instruction for using -func with -o. Is that
fine?

https://codereview.appspot.com/13532052/diff/31001/cmd/cover/cover.go#newcode60
cmd/cover/cover.go:60: Name of coverage variable to generate
html & func are appropriately excluded from this list, and the flags are
indented to be part of this usage scenario.

a little more work when adding a Flag, but IMO better results

[r, 2013-09-24 02:48:17]

Please let me know when it's ready for another look.

-rob

[nathany, 2013-09-24 02:54:36]

On 2013/09/24 02:48:17, r wrote:
> Please let me know when it's ready for another look.
> 
> -rob

Please do. Thanks.

[r, 2013-09-24 03:29:50]

https://codereview.appspot.com/13532052/diff/27001/cmd/cover/cover.go
File cmd/cover/cover.go (right):

https://codereview.appspot.com/13532052/diff/27001/cmd/cover/cover.go#newcode38
cmd/cover/cover.go:38: Given a coverage profile produced by 'go test':
s/:/, for example by running:

https://codereview.appspot.com/13532052/diff/27001/cmd/cover/cover.go#newcode60
cmd/cover/cover.go:60: Name of coverage variable to generate
you don't need to go through this because humans never run this. it's for go
test only.

The cover tool is used by "go test -cover" to to rewrite source code with
coverage annotations with an invocation such as
      go tool cover -mode=set -var=CoverageVariableName program.go
These flags are documented separately. Such processing happens automatically;
users do not need to run the tool this way.

[nathany, 2013-09-24 03:47:03]

https://codereview.appspot.com/13532052/diff/27001/cmd/cover/cover.go
File cmd/cover/cover.go (right):

https://codereview.appspot.com/13532052/diff/27001/cmd/cover/cover.go#newcode60
cmd/cover/cover.go:60: Name of coverage variable to generate
On 2013/09/24 03:29:50, r wrote:
> you don't need to go through this because humans never run this. it's for go
> test only.

I just wanted the flags indented as part of this section. Also splitting the
flags up into two groups, with html & func documented above and not here.

> The cover tool is used by "go test -cover" to to rewrite source code with
> coverage annotations with an invocation such as
>       go tool cover -mode=set -var=CoverageVariableName program.go
> These flags are documented separately. Such processing happens automatically;
> users do not need to run the tool this way.

[nathany, 2013-09-24 04:09:34]

I reduced the indentation to align with the flags below, though it still
includes func & html in that section.

Perhaps a better option is to move the "source code rewriting" help under
another option, eg. go tool cover -help -v.

Then the usage information for most people could end something like this:

See 'go help testflag' for details on producing a coverage profile.

Cover is used by 'go test -cover' to rewrite source code with coverage
annotations. See 'go tool cover -help -v' for advanced usage.

If you like that option, do you have a suggestion for the advanced usage flag?

[r, 2013-09-24 04:16:25]

Please let's just bring this to ground in the form I suggested.

-rob

[nathany, 2013-09-24 04:32:33]

On 2013/09/24 04:16:25, r wrote:
> Please let's just bring this to ground in the form I suggested.
> 
> -rob

Okay. Thanks for your patience with me. I hope the 10th one's the charm.

---
Usage of 'go tool cover':
Given a coverage profile produced by 'go test':
	go test -coverprofile=c.out

Open a web browser displaying annotated source code:
	go tool cover -html=c.out

Write out an HTML file instead of launching a web browser:
	go tool cover -html=c.out -o coverage.html

Display coverage percentages to stdout for each function:
	go tool cover -func=c.out

Or to generate modified source code with coverage annotations (advanced usage):
	go tool cover -mode=set -var=CoverageVariableName program.go

Flags:
  -func="": output coverage profile information for each function
  -html="": generate HTML representation of coverage profile
  -mode="": coverage mode: set, count, atomic
  -o="": file for output; default: stdout
  -var="GoCover": name of coverage variable to generate

  Only one of -html, -func, or -mode may be set.

[r, 2013-09-24 05:38:51]

LGTM

https://codereview.appspot.com/13532052/diff/34001/cmd/cover/cover.go
File cmd/cover/cover.go (right):

https://codereview.appspot.com/13532052/diff/34001/cmd/cover/cover.go#newcode49
cmd/cover/cover.go:49: Or to generate modified source code with coverage
annotations (advanced usage):
it's not advanced usage. it's never used usage. this is for the go tool. please
see the text i proposed last time.

the rest is looking like a genuine improvement.

[nathany, 2013-09-24 06:41:05]

On 2013/09/24 05:38:51, r wrote:
> LGTM
> 
> https://codereview.appspot.com/13532052/diff/34001/cmd/cover/cover.go
> File cmd/cover/cover.go (right):
> 
>
https://codereview.appspot.com/13532052/diff/34001/cmd/cover/cover.go#newcode49
> cmd/cover/cover.go:49: Or to generate modified source code with coverage
> annotations (advanced usage):
> it's not advanced usage. it's never used usage. this is for the go tool.
please
> see the text i proposed last time.
> 
> the rest is looking like a genuine improvement.

Thanks. Though this could've went a lot faster if I learned to listen better!

Can we call it done?

Finally, to generate modified source code with coverage annotations
(what go test -cover does):
	go tool cover -mode=set -var=CoverageVariableName program.go

--
Tomorrow the "flag provided but not defined" bit.

[r, 2013-09-24 07:27:36]

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

go.tools/cover: clean up usage information

R=golang-dev, r, minux.ma
CC=golang-dev
https://codereview.appspot.com/13532052

Committer: Rob Pike <r@golang.org>

[nathany, 2013-10-27 18:23:09]

*** Abandoned ***