code review 2985042: http: Add EncodeQuery, a handy helper function for cons...

http: Add EncodeQuery, a handy helper function for constructing URL query strings.

[dsymonds, 2010-11-09 22:39:56]

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

I'd like you to review this change.

[rsc, 2010-11-15 21:52:40]

Please merge with urlencode.

Russ

[dsymonds, 2010-11-15 22:08:35]

On Tue, Nov 16, 2010 at 8:52 AM, Russ Cox <rsc@golang.org> wrote:

> Please merge with urlencode.

Done. I didn't want to leap to changing the PostForm signature, so I
left urlencode as a little wrapper between the map/multimap form. Let
me know if you want something different.

[dsymonds, 2010-11-23 23:44:23]

ping

[rsc, 2010-11-24 01:22:37]

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

http://codereview.appspot.com/2985042/diff/7001/src/pkg/http/url.go#newcode522
src/pkg/http/url.go:522: func EncodeQuery(m map[string][]interface{}) string {
I'm not okay with this.  Why interface{}?  What values are okay?  Etc.

I think this should be map[string]string to match PostForm.
I know that's not the right long term type, but when we switch
PostForm we can switch this one to (to map[string][]string).

[dsymonds, 2010-11-24 01:54:03]

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

http://codereview.appspot.com/2985042/diff/7001/src/pkg/http/url.go#newcode522
src/pkg/http/url.go:522: func EncodeQuery(m map[string][]interface{}) string {
On 2010/11/24 01:22:38, rsc wrote:
> I'm not okay with this.  Why interface{}?  What values are okay?  Etc.

It's interface{} in the same way that fmt.Print, etc. take interface{}. Any
value that fmt can render should be usable here, though it will usually be
primitive types.

I'm trying to make it easier to build URLs, and plenty of URLs have query
parameter values that are semantically strings, ints, etc. I could change the
interface{} to string, but that just shifts the fmt.Sprint burden back to all
the callers, which seems silly given that this is mean to be a convenience
function.

[rsc, 2010-11-24 02:40:07]

>> I'm not okay with this.  Why interface{}?  What values are okay?  Etc.
>
> It's interface{} in the same way that fmt.Print, etc. take interface{}.
> Any value that fmt can render should be usable here, though it will
> usually be primitive types.
>
> I'm trying to make it easier to build URLs, and plenty of URLs have
> query parameter values that are semantically strings, ints, etc. I could
> change the interface{} to string, but that just shifts the fmt.Sprint
> burden back to all the callers, which seems silly given that this is
> mean to be a convenience function.

I'm not convinced the convenience here outweighs
the possibility of putting something in the map that
you didn't intend and silently sending key=0x3749554.
I'd rather have the explicit conversions, as in the rest
of the language.  Since the caller has to create
the map anyway (there aren't map[string][]interface{}
lying around already) it seems reasonable to give the
caller the responsibility for the conversions.  I think
what you have would be a typical Python API; it isn't
a Go API.

Also, we have two representations of query params now.
ParseForm expects map[string]string.
Request.Form is a map[string][]string.
The goal is to reduce to one (I believe it will be
map[string][]string), not grow to three.
EncodeQuery should take one of those two,
either map[string]string to match ParseForm now
but get changed later, or map[string][]string to
match Request.Form now and everything later.

Russ

[rsc, 2010-11-24 02:43:38]

s/ParseForm/PostForm/ in my last mail

[dsymonds, 2010-11-24 23:07:46]

Okay, you won me over. PTAL.

[dsymonds, 2010-12-07 03:29:21]

ping

[rsc, 2010-12-07 17:56:52]

LGTM

[dsymonds, 2010-12-07 20:43:05]

Can you submit this?

[rsc, 2010-12-07 20:54:08]

*** Submitted as f25afbb6be5a ***

http: Add EncodeQuery, a handy helper function for constructing URL query
strings.

R=stephenm, rsc
CC=golang-dev
http://codereview.appspot.com/2985042

Committer: Russ Cox <rsc@golang.org>