code review 13956043: go.tools/cover: validate flags with appropriate error m...

go.tools/cover: validate flags with appropriate error messages

Fixes issue 6444.

[nathany, 2013-09-26 06:13:40]

Hello golang-dev@googlegroups.com (cc: r@golang.org),

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

[r, 2013-09-26 06:43:57]

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

https://codereview.appspot.com/13956043/diff/6001/cmd/cover/cover.go#newcode97
cmd/cover/cover.go:97: cover(flag.Arg(0))
return

then drop the next else. it's really two different things. get one of them done.

https://codereview.appspot.com/13956043/diff/6001/cmd/cover/cover.go#newcode109
cmd/cover/cover.go:109: // validateFlags reports invalid argument errors and
sets up some globals
it doesn't report them it just returns an error.

// validateFlags sets up the state of the program based on the flags.
// It returns an error if the flags are in correct.

but notice that the purpose of this is to set up state; the validation is
secondary. it's misnamed. it's like calling file.Open ValidateFile.

https://codereview.appspot.com/13956043/diff/6001/cmd/cover/cover.go#newcode144
cmd/cover/cover.go:144: return fmt.Errorf("unexpected argument provided")
this isn't quite right.  "extra argument"? "too many arguments"?

[nathany, 2013-09-26 06:58:50]

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

https://codereview.appspot.com/13956043/diff/6001/cmd/cover/cover.go#newcode109
cmd/cover/cover.go:109: // validateFlags reports invalid argument errors and
sets up some globals
On 2013/09/26 06:43:57, r wrote:
> it doesn't report them it just returns an error.
> 
> // validateFlags sets up the state of the program based on the flags.
> // It returns an error if the flags are in correct.
> 
> but notice that the purpose of this is to set up state; the validation is
> secondary. it's misnamed. it's like calling file.Open ValidateFile.

I could rename it (parseFlags, i don't know?) or change it to only do
validation. 

The global profile var is completely unnecessary right now. I could setup
counterStmt from main or in generateCoverage(program, mode string) function. I'd
like to still validate mode upfront though, which makes for an additional case
"set", "count", "atomic" statement (two places to change). Am I over thinking
it? :-)

[r, 2013-09-26 07:01:21]

parseFlags sounds right.

[nathany, 2013-09-26 07:08:07]

Hello golang-dev@googlegroups.com, r@golang.org (cc:
golang-dev@googlegroups.com),

Please take another look.

[r, 2013-09-26 07:59:26]

starting to look good

https://codereview.appspot.com/13956043/diff/17001/cmd/cover/cover.go
File cmd/cover/cover.go (right):

https://codereview.appspot.com/13956043/diff/17001/cmd/cover/cover.go#newcode88
cmd/cover/cover.go:88: if err := parseFlags(); err != nil {
it's a big step and you need an err later, so break this into two lines.
err := parseFlags()
if err != nil {

https://codereview.appspot.com/13956043/diff/17001/cmd/cover/cover.go#newcode119
cmd/cover/cover.go:119: return fmt.Errorf("provide one of -html, -func, or
-mode")
phrasing: say what's wrong, not how to fix it.

too many options

https://codereview.appspot.com/13956043/diff/17001/cmd/cover/cover.go#newcode142
cmd/cover/cover.go:142: return fmt.Errorf("provide a source file to annotate")
ditto:

missing source file

https://codereview.appspot.com/13956043/diff/17001/cmd/cover/cover.go#newcode149
cmd/cover/cover.go:149: return fmt.Errorf("too many arguments provided")
s/ provided//  it's redundant.

[nathany, 2013-09-26 08:08:33]

On 2013/09/26 07:59:26, r wrote:
> starting to look good

thanks for the code review. please take another look.

it's 2 AM here, so if there are any other changes, I'll make them tomorrow
evening.

take care.

[r, 2013-09-26 08:09:36]

LGTM

[r, 2013-09-26 08:11:14]

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

go.tools/cover: validate flags with appropriate error messages

Fixes issue 6444.

R=golang-dev, r
CC=golang-dev
https://codereview.appspot.com/13956043

Committer: Rob Pike <r@golang.org>