code review 4410041: expvar: add ValueFunc for variables that are JSON marsh...

expvar: add Func for functions that return values that are JSON marshalable.

Remove {Float,Int,String}Func, which are now redundant.

Fixes issue 1684.

[dsymonds, 2011-04-13 06:27:54]

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

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

[r, 2011-04-13 20:43:16]

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

http://codereview.appspot.com/4410041/diff/3001/src/pkg/expvar/expvar.go#newc...
src/pkg/expvar/expvar.go:201: // ValueFunc wraps a func() interface{} that
returns a value that can be
i don't understand your use of the word 'wrap'.

// ValueFunc is the type of a function that returns a value that can be
// serialized as JSON.

why isn't it called JSON or JSONFunc?

[rsc, 2011-04-13 20:50:20]

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

http://codereview.appspot.com/4410041/diff/3001/src/pkg/expvar/expvar.go#newc...
src/pkg/expvar/expvar.go:201: // ValueFunc wraps a func() interface{} that
returns a value that can be
On 2011/04/13 20:43:16, r wrote:
> i don't understand your use of the word 'wrap'.

i agree but we need to change all of them then

> // ValueFunc is the type of a function that returns a value that can be
> // serialized as JSON.
> 
> why isn't it called JSON or JSONFunc?

the function doesn't return JSON.
it just needs to return a value that package json can handle.

on balance i think calling it JSONFunc would encourage people
to return json-formatted strings, which would then get re-encoded.

how about just Func?

[dsymonds, 2011-04-13 23:19:41]

Func works for me, except that we'd be painted into a corner if we later
want a type that holds an interface{} and provides atomic access, like
Int/String/etc.

[r2, 2011-04-13 23:21:20]

On Apr 13, 2011, at 4:19 PM, David Symonds wrote:

> Func works for me, except that we'd be painted into a corner if we later want
a type that holds an interface{} and provides atomic access, like Int/String/etc

The paint will dry and we can walk out.

-rob

[dsymonds, 2011-04-14 00:01:59]

PTAL

[r, 2011-04-14 00:07:33]

http://codereview.appspot.com/4410041/diff/10001/src/pkg/expvar/expvar.go
File src/pkg/expvar/expvar.go (right):

http://codereview.appspot.com/4410041/diff/10001/src/pkg/expvar/expvar.go#new...
src/pkg/expvar/expvar.go:208: }
i'm not happy with the name. sorry.
the problem i have is that from the signature and method, i don't know how to
use it.
if you document String(), does godoc show it? if so, that's where you can
explain what it does.

down below, at the call site Func(cmdline) is a very strange thing to read.
(so was ValueFunc(cmdline)).
rsc pushed back but i do think you want JSON in there.
maybe even JSON; perhaps ToJSON?

then the call site looks like

Publish("memstats", JSON(memstats))

now i understand what it does at the call site.

[dsymonds, 2011-04-15 02:09:06]

I've left the comment on the type, since that's where all the other ones are.
Part of the reason for wanting it to be named *Func was that it would
be consistent with the others.

Anyway, it's named JSON now. PTAL.

[r, 2011-04-15 02:58:11]

LGTM
but let rsc agree - he wasn't happy before

http://codereview.appspot.com/4410041/diff/13001/src/pkg/expvar/expvar.go
File src/pkg/expvar/expvar.go (right):

http://codereview.appspot.com/4410041/diff/13001/src/pkg/expvar/expvar.go#new...
src/pkg/expvar/expvar.go:201: // JSON wraps a func() interface{} that returns a
value that can be
s/wraps/identifies/
there's no wrapping going on

[rsc, 2011-04-15 03:28:07]

This still doesn't seem right.
I propose deleting all the Funcs and having only

// Func implements Var by calling the function each time
// the value is needed and formatting the returned value using JSON.
type Func func() interface{}

func (f Func) String() string {
    ...
}

[r2, 2011-04-15 03:32:53]

On Apr 14, 2011, at 8:28 PM, Russ Cox wrote:

> This still doesn't seem right.
> I propose deleting all the Funcs and having only
> 
> // Func implements Var by calling the function each time
> // the value is needed and formatting the returned value using JSON.
> type Func func() interface{}
> 
> func (f Func) String() string {
>    ...
> }

that sounds good

[dsymonds, 2011-04-15 04:00:27]

PTAL

[rsc, 2011-04-15 04:13:02]

LGTM

[dsymonds, 2011-04-15 08:21:33]

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

expvar: add Func for functions that return values that are JSON marshalable.

Remove {Float,Int,String}Func, which are now redundant.

Fixes issue 1684.

R=rsc, r, r2
CC=golang-dev
http://codereview.appspot.com/4410041