code review 1973043: fmt/print: honor Formatter in Print, Println.

fmt/print: honor Formatter in Print, Println.
Rearrange code to clarify handling of %T, Formatter, GoStringer, and Stringer.

[r, 2010-08-13 03:42:41]

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

I'd like you to review this change.

[rsc, 2010-08-13 03:47:07]

I think the check belongs in printField rather than the
two places it is now.  Try printing a struct or a map
or a slice with a formatted type inside it.

Russ

[r2, 2010-08-13 03:58:27]

PTAL

[rsc1, 2010-08-13 04:10:14]

http://codereview.appspot.com/1973043/diff/5001/6002
File src/pkg/fmt/print.go (right):

http://codereview.appspot.com/1973043/diff/5001/6002#newcode499
src/pkg/fmt/print.go:499: // Stringer?
I think this is converging on something cleaner
but not quite there.

The old sequence of operations was:

if Formatter, use it.
if goSyntax, use GoString or reflect
if !goSyntax, use String or reflect

This code is different: Formatter is 
now a last resort.  That doesn't seem right.

I think you want to check Formatter first,
and then the if plus { p.fmt.plus = false }
can happen before everything else.
I also think that changing the switch into
an if !goSyntax will lead to a clearer
statement of the code.

Finally, and sorry for pointing this out but
maybe *it* will have some bearing on issue 1024,
the handling of GoStringer seems wrong too,
because it recurses back into printField with
a string and goSyntax==true, which will quote
the string.  So if the GoStringer returns 
`[]int{1,2,3}` what gets printed is `"[]int{1,2,3}"`.

[r, 2010-08-13 05:43:19]

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

Please take another look.

[rsc1, 2010-08-13 06:34:46]

still thinking about Stringer but can get this out of the way

http://codereview.appspot.com/1973043/diff/13001/14002
File src/pkg/fmt/print.go (right):

http://codereview.appspot.com/1973043/diff/13001/14002#newcode497
src/pkg/fmt/print.go:497: if field != nil {
Seeing this test confuses me as a reader.
I think everything here works (falls through without doing anything) when field
== nil.  I also think that the clearing
of plus and sharp can happen even if field == nil.
So the if can go away, dropping some indentation.

Maybe it also makes sense to bring the if field == nil on line 597 up here and
get it over with.

[r2, 2010-08-13 06:39:38]

On Aug 13, 2010, at 4:34 PM, rsc@google.com wrote:

> still thinking about Stringer but can get this out of the way
>
>
>
> http://codereview.appspot.com/1973043/diff/13001/14002
> File src/pkg/fmt/print.go (right):
>
> http://codereview.appspot.com/1973043/diff/13001/14002#newcode497
> src/pkg/fmt/print.go:497: if field != nil {
> Seeing this test confuses me as a reader.
> I think everything here works (falls through without doing anything)
> when field == nil.  I also think that the clearing
> of plus and sharp can happen even if field == nil.
> So the if can go away, dropping some indentation.

the tests pass and it looks better, but i am skeptical. let's try it  
though.

> Maybe it also makes sense to bring the if field == nil on line 597 up
> here and get it over with.

sure.

PTAL

[r2, 2010-08-13 06:42:30]

On Aug 13, 2010, at 4:34 PM, rsc@google.com wrote:

> still thinking about Stringer but can get this out of the way

i want the verb to be used when printing with Stringer.  i think it  
has to recur.

i moved the nil test to the very top.  that makes it safe as well as  
test-passy.

-rob

[rsc1, 2010-08-13 06:52:50]

http://codereview.appspot.com/1973043/diff/13001/14002
File src/pkg/fmt/print.go (right):

http://codereview.appspot.com/1973043/diff/13001/14002#newcode496
src/pkg/fmt/print.go:496: // Special processing considerations.
One more suggestion: move the %T test here too.
Then all the special stuff is together:

if field == nil ...
if formatter, ok ...
p.fmt.plus = false
switch {
case c == 'T': ...
case c == 'p': ...
case goSyntax:
    if gostringer ...
default:
    if stringer ...
}

Phrased like that I can see that %p on a *T
is a different situation than %d on a Stringer
so I'd lean toward taking issue 1024 as a real bug.

[r2, 2010-08-13 07:13:43]

On Aug 13, 2010, at 4:52 PM, rsc@google.com wrote:

>
> http://codereview.appspot.com/1973043/diff/13001/14002
> File src/pkg/fmt/print.go (right):
>
> http://codereview.appspot.com/1973043/diff/13001/14002#newcode496
> src/pkg/fmt/print.go:496: // Special processing considerations.
> One more suggestion: move the %T test here too.
> Then all the special stuff is together:
>
> if field == nil ...
> if formatter, ok ...
> p.fmt.plus = false
> switch {
> case c == 'T': ...
> case c == 'p': ...
> case goSyntax:
>    if gostringer ...
> default:
>    if stringer ...
> }

i think %T needs to be before formatter.  before everything but nil,  
really.  at this point i'm prepared to make the same argument 'p'.
look at the current state and see if you agree.

> Phrased like that I can see that %p on a *T
> is a different situation than %d on a Stringer
> so I'd lean toward taking issue 1024 as a real bug.

>
> http://codereview.appspot.com/1973043/show

[rsc1, 2010-08-13 07:17:50]

LGTM

http://codereview.appspot.com/1973043/diff/7003/29002
File src/pkg/fmt/print.go (right):

http://codereview.appspot.com/1973043/diff/7003/29002#newcode497
src/pkg/fmt/print.go:497: if verb == 'v' {
verb == 'v' || verb == 'T'

(matches old behavior)

[r, 2010-08-13 07:26:40]

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

fmt/print: honor Formatter in Print, Println.
Rearrange code to clarify handling of %T, Formatter, GoStringer, and Stringer.

R=rsc
CC=golang-dev
http://codereview.appspot.com/1973043