code review 5564065: flag: allow a FlagSet to not write to os.Stderr

flag: allow a FlagSet to not write to os.Stderr

Fixes issue 2747

[bradfitz, 2012-01-27 01:15:55]

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

I'd like you to review this change to
https://go.googlecode.com/hg/

[gri, 2012-01-27 01:32:21]

FYI.

r should weigh in.

http://codereview.appspot.com/5564065/diff/3001/src/pkg/flag/flag.go
File src/pkg/flag/flag.go (right):

http://codereview.appspot.com/5564065/diff/3001/src/pkg/flag/flag.go#newcode227
src/pkg/flag/flag.go:227: Output io.Writer
I'd make this private and get rid of the accessor below. A new FlagSet needs to
be created with NewFlagSet or initialized w/ Init so the zero value of FlagSet
is not valid. Init can set output to os.Stderr.

Instead I would provide a SetOutput (or similar) function to change the output.
Seems cleaner - there are no other exported fields here.

http://codereview.appspot.com/5564065/diff/3001/src/pkg/flag/flag.go#newcode332
src/pkg/flag/flag.go:332: func (fs *FlagSet) PrintDefaults() {
I'd leave this alone (s/fs/f/) for consistency w/ all other methods, and instead
do s/f/flag/ below.

[bradfitz, 2012-01-27 01:37:16]

FlagSet.Init refers to "the zero FlagSet", suggesting it's usable on its
own, and shouldn't crash if output is nil, hence the accessor.

Init is already kinda weird.  I don't want to add a third parameter there,
but having one function named Init setting two things and a second doing
doing its own single field seems weird.

Waiting for Rob.


On Thu, Jan 26, 2012 at 5:32 PM, <gri@golang.org> wrote:

> FYI.
>
> r should weigh in.
>
>
>
http://codereview.appspot.com/**5564065/diff/3001/src/pkg/**flag/flag.go<http...
> File src/pkg/flag/flag.go (right):
>
> http://codereview.appspot.com/**5564065/diff/3001/src/pkg/**
>
flag/flag.go#newcode227<http://codereview.appspot.com/5564065/diff/3001/src/pkg/flag/flag.go#newcode227>
> src/pkg/flag/flag.go:227: Output io.Writer
> I'd make this private and get rid of the accessor below. A new FlagSet
> needs to be created with NewFlagSet or initialized w/ Init so the zero
> value of FlagSet is not valid. Init can set output to os.Stderr.
>
> Instead I would provide a SetOutput (or similar) function to change the
> output. Seems cleaner - there are no other exported fields here.
>
> http://codereview.appspot.com/**5564065/diff/3001/src/pkg/**
>
flag/flag.go#newcode332<http://codereview.appspot.com/5564065/diff/3001/src/pkg/flag/flag.go#newcode332>
> src/pkg/flag/flag.go:332: func (fs *FlagSet) PrintDefaults() {
> I'd leave this alone (s/fs/f/) for consistency w/ all other methods, and
> instead do s/f/flag/ below.
>
>
http://codereview.appspot.com/**5564065/<http://codereview.appspot.com/5564065/>
>

[r, 2012-01-27 02:54:55]

http://codereview.appspot.com/5564065/diff/3001/src/pkg/flag/flag.go
File src/pkg/flag/flag.go (right):

http://codereview.appspot.com/5564065/diff/3001/src/pkg/flag/flag.go#newcode227
src/pkg/flag/flag.go:227: Output io.Writer
some of the above. the zero value should be usable.

1) do not export the field
2) keep the private accessor
3) provide a SetOutput method.

http://codereview.appspot.com/5564065/diff/3001/src/pkg/flag/flag.go#newcode239
src/pkg/flag/flag.go:239: if f.Output != nil {
i'd flip this, put the special case first and do == nil here.

[rog, 2012-01-27 11:31:53]

LGTM modulo rob's comments.
no test?

On 27 January 2012 02:54,  <r@golang.org> wrote:
>
> http://codereview.appspot.com/5564065/diff/3001/src/pkg/flag/flag.go
> File src/pkg/flag/flag.go (right):
>
>
http://codereview.appspot.com/5564065/diff/3001/src/pkg/flag/flag.go#newcode227
> src/pkg/flag/flag.go:227: Output io.Writer
> some of the above. the zero value should be usable.
>
> 1) do not export the field
> 2) keep the private accessor
> 3) provide a SetOutput method.
>
>
http://codereview.appspot.com/5564065/diff/3001/src/pkg/flag/flag.go#newcode239
> src/pkg/flag/flag.go:239: if f.Output != nil {
> i'd flip this, put the special case first and do == nil here.
>
> http://codereview.appspot.com/5564065/

[r2, 2012-01-27 16:12:53]

On Jan 26, 2012, at 5:37 PM, Brad Fitzpatrick wrote:

> FlagSet.Init refers to "the zero FlagSet", suggesting it's usable on its own,
and shouldn't crash if output is nil, hence the accessor.
> 
> Init is already kinda weird.  I don't want to add a third parameter there, but
having one function named Init setting two things and a second doing doing its
own single field seems weird.

it does, but this is such a special feature almost no one will use it. it
doesn't bother me to have it tucked away.

-rob

[bradfitz, 2012-01-27 16:25:08]

Hello golang-dev@googlegroups.com, gri@golang.org, r@golang.org,
rogpeppe@gmail.com, r@google.com (cc: golang-dev@googlegroups.com),

Please take another look.

[bradfitz, 2012-01-27 16:48:18]

All addressed, and added a test.

On Fri, Jan 27, 2012 at 3:31 AM, roger peppe <rogpeppe@gmail.com> wrote:

> LGTM modulo rob's comments.
> no test?
>
> On 27 January 2012 02:54,  <r@golang.org> wrote:
> >
> > http://codereview.appspot.com/5564065/diff/3001/src/pkg/flag/flag.go
> > File src/pkg/flag/flag.go (right):
> >
> >
>
http://codereview.appspot.com/5564065/diff/3001/src/pkg/flag/flag.go#newcode227
> > src/pkg/flag/flag.go:227: Output io.Writer
> > some of the above. the zero value should be usable.
> >
> > 1) do not export the field
> > 2) keep the private accessor
> > 3) provide a SetOutput method.
> >
> >
>
http://codereview.appspot.com/5564065/diff/3001/src/pkg/flag/flag.go#newcode239
> > src/pkg/flag/flag.go:239: if f.Output != nil {
> > i'd flip this, put the special case first and do == nil here.
> >
> > http://codereview.appspot.com/5564065/
>

[r, 2012-01-27 17:07:26]

LGTM

http://codereview.appspot.com/5564065/diff/7001/src/pkg/flag/flag.go
File src/pkg/flag/flag.go (right):

http://codereview.appspot.com/5564065/diff/7001/src/pkg/flag/flag.go#newcode791
src/pkg/flag/flag.go:791: // SetOutput sets where usage and error messages are
written.
SetOutput sets the destination for usage and error messages.

http://codereview.appspot.com/5564065/diff/7001/src/pkg/flag/flag.go#newcode792
src/pkg/flag/flag.go:792: // If output is nil, os.Stderr is used.
put this right after out(), so the two methods for the field are together.

http://codereview.appspot.com/5564065/diff/7001/src/pkg/flag/flag_test.go
File src/pkg/flag/flag_test.go (right):

http://codereview.appspot.com/5564065/diff/7001/src/pkg/flag/flag_test.go#new...
src/pkg/flag/flag_test.go:218: t.Logf("expected output mentionining unknown; got
%q", out)
s/mentionining/mentioning/

[bradfitz, 2012-01-27 17:23:09]

*** Submitted as http://code.google.com/p/go/source/detail?r=5d72030a6c50 ***

flag: allow a FlagSet to not write to os.Stderr

Fixes issue 2747

R=golang-dev, gri, r, rogpeppe, r
CC=golang-dev
http://codereview.appspot.com/5564065