[golang-dev] Re: code review 9671043: google-api-go-client: avoid reencoding parameters in th... (issue 9671043)

Adg

at May 22, 2013 at 11:24 pm

http://golang.org/pkg/net/url/#QueryEscape ?

https://codereview.appspot.com/9671043/

--

---
You received this message because you are subscribed to the Google Groups "golang-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [email protected].
For more options, visit https://groups.google.com/groups/opt_out.

reply | permalink

Adg

at May 22, 2013 at 11:28 pm

⇧

I'd like to see all the changes to generated code in this CL, too. Not
sure if that's what brad has done in the past also.

https://codereview.appspot.com/9671043/diff/12001/google-api-go-generator/gen.go
File google-api-go-generator/gen.go (right):

https://codereview.appspot.com/9671043/diff/12001/google-api-go-generator/gen.go#newcode1150
google-api-go-generator/gen.go:1150: // E.g. Cloud Storage API requires
'%2F' in entity param to be kept, but url.Parse replaces it by '/'.
s/by/with/

https://codereview.appspot.com/9671043/diff/12001/google-api-go-generator/gen.go#newcode1152
google-api-go-generator/gen.go:1152: p("\treq.URL.Path =
strings.Replace(req.URL.Path, \"{%s}\", %s, 1)\n", arg.apiname,
arg.cleanExpr("c."))
use pn(`req.URL ...`, ...)

https://codereview.appspot.com/9671043/diff/12001/google-api-go-generator/gen.go#newcode1155
google-api-go-generator/gen.go:1155: p("\treq.URL.Opaque =
req.URL.Path\n")
pn()

https://codereview.appspot.com/9671043/

--

---
You received this message because you are subscribed to the Google Groups "golang-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [email protected].
For more options, visit https://groups.google.com/groups/opt_out.

reply | permalink

Brad Fitzpatrick

at May 22, 2013 at 11:35 pm

⇧

On Wed, May 22, 2013 at 4:28 PM, wrote:

I'd like to see all the changes to generated code in this CL, too. Not
sure if that's what brad has done in the past also.

Agree.

--

---
You received this message because you are subscribed to the Google Groups "golang-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [email protected].
For more options, visit https://groups.google.com/groups/opt_out.

reply | permalink

Campoy

at May 22, 2013 at 11:39 pm

⇧

All the changes are here: https://codereview.appspot.com/9672043/

https://codereview.appspot.com/9671043/diff/12001/google-api-go-generator/gen.go
File google-api-go-generator/gen.go (right):

https://codereview.appspot.com/9671043/diff/12001/google-api-go-generator/gen.go#newcode1150
google-api-go-generator/gen.go:1150: // E.g. Cloud Storage API requires
'%2F' in entity param to be kept, but url.Parse replaces it by '/'.

On 2013/05/22 23:28:55, adg wrote:
s/by/with/

Done.

https://codereview.appspot.com/9671043/diff/12001/google-api-go-generator/gen.go#newcode1152
google-api-go-generator/gen.go:1152: p("\treq.URL.Path =
strings.Replace(req.URL.Path, \"{%s}\", %s, 1)\n", arg.apiname,
arg.cleanExpr("c."))

On 2013/05/22 23:28:55, adg wrote:
use pn(`req.URL ...`, ...)

Done.

https://codereview.appspot.com/9671043/diff/12001/google-api-go-generator/gen.go#newcode1155
google-api-go-generator/gen.go:1155: p("\treq.URL.Opaque =
req.URL.Path\n")

On 2013/05/22 23:28:55, adg wrote:
pn()

Done.

https://codereview.appspot.com/9671043/

--

---
You received this message because you are subscribed to the Google Groups "golang-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [email protected].
For more options, visit https://groups.google.com/groups/opt_out.

reply | permalink

Brad Fitzpatrick

at May 22, 2013 at 11:45 pm

⇧

is this really the right use of req.URL.Opaque?

I remember knowing this once, but it feels wrong and doesn't seem to match
the docs at http://golang.org/pkg/net/url/#URL

On Wed, May 22, 2013 at 4:39 PM, wrote:

All the changes are here: https://codereview.appspot.**com/9672043/<https://codereview.appspot.com/9672043/>

https://codereview.appspot.**com/9671043/diff/12001/google-**
api-go-generator/gen.go<https://codereview.appspot.com/9671043/diff/12001/google-api-go-generator/gen.go>
File google-api-go-generator/gen.go (right):

https://codereview.appspot.**com/9671043/diff/12001/google-**
api-go-generator/gen.go#**newcode1150<https://codereview.appspot.com/9671043/diff/12001/google-api-go-generator/gen.go#newcode1150>
google-api-go-generator/gen.**go:1150: // E.g. Cloud Storage API requires
'%2F' in entity param to be kept, but url.Parse replaces it by '/'.

On 2013/05/22 23:28:55, adg wrote:

s/by/with/

Done.

https://codereview.appspot.**com/9671043/diff/12001/google-**
api-go-generator/gen.go#**newcode1152<https://codereview.appspot.com/9671043/diff/12001/google-api-go-generator/gen.go#newcode1152>
google-api-go-generator/gen.**go:1152: p("\treq.URL.Path =
strings.Replace(req.URL.Path, \"{%s}\", %s, 1)\n", arg.apiname,
arg.cleanExpr("c."))

On 2013/05/22 23:28:55, adg wrote:

use pn(`req.URL ...`, ...)

Done.

https://codereview.appspot.**com/9671043/diff/12001/google-**
api-go-generator/gen.go#**newcode1155<https://codereview.appspot.com/9671043/diff/12001/google-api-go-generator/gen.go#newcode1155>
google-api-go-generator/gen.**go:1155: p("\treq.URL.Opaque =
req.URL.Path\n")

On 2013/05/22 23:28:55, adg wrote:

pn()

Done.

https://codereview.appspot.**com/9671043/<https://codereview.appspot.com/9671043/>

--

---
You received this message because you are subscribed to the Google Groups "golang-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [email protected].
For more options, visit https://groups.google.com/groups/opt_out.

reply | permalink

Andrew Gerrand

at May 22, 2013 at 11:52 pm

⇧

On 23 May 2013 09:45, Brad Fitzpatrick wrote:

is this really the right use of req.URL.Opaque?

I don't think it is. Used in this way, would drop the host from the request
URL:

http://play.golang.org/p/xn0SO9y2mC

--

---
You received this message because you are subscribed to the Google Groups "golang-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [email protected].
For more options, visit https://groups.google.com/groups/opt_out.

reply | permalink

Adg

at May 23, 2013 at 12:28 am

⇧

https://codereview.appspot.com/9671043/diff/17001/google-api-go-generator/gen.go
File google-api-go-generator/gen.go (right):

https://codereview.appspot.com/9671043/diff/17001/google-api-go-generator/gen.go#newcode1155
google-api-go-generator/gen.go:1155: pn("req.URL.Opaque =
req.URL.Path\n")
I think this should be

pn(`req.URL.Opaque = "//" + req.URL.Host + req.URL.Path`)

https://codereview.appspot.com/9671043/

--

---
You received this message because you are subscribed to the Google Groups "golang-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [email protected].
For more options, visit https://groups.google.com/groups/opt_out.

reply | permalink

Campoy

at May 23, 2013 at 12:41 am

⇧

I tested and it works with your solution too.

Opaque fields are opaque ...

https://codereview.appspot.com/9671043/diff/17001/google-api-go-generator/gen.go
File google-api-go-generator/gen.go (right):

https://codereview.appspot.com/9671043/diff/17001/google-api-go-generator/gen.go#newcode1155
google-api-go-generator/gen.go:1155: pn("req.URL.Opaque =
req.URL.Path\n")

On 2013/05/23 00:27:59, adg wrote:
I think this should be
pn(`req.URL.Opaque = "//" + req.URL.Host + req.URL.Path`)

Done.

https://codereview.appspot.com/9671043/

--

---
You received this message because you are subscribed to the Google Groups "golang-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [email protected].
For more options, visit https://groups.google.com/groups/opt_out.

reply | permalink

Adg

at May 23, 2013 at 12:45 am

⇧

I'm a bit worried about your tests if it worked before. The way you had
it would have clobbered the hostname, making the url unusable.

https://codereview.appspot.com/9671043/diff/19002/google-api-go-generator/gen.go
File google-api-go-generator/gen.go (right):

https://codereview.appspot.com/9671043/diff/19002/google-api-go-generator/gen.go#newcode1152
google-api-go-generator/gen.go:1152: pn("req.URL.Path =
strings.Replace(req.URL.Path, \"{%s}\", %s, 1)\n", arg.apiname,
arg.cleanExpr("c."))
AGAIN, please use `backtics` to avoid the needless quoting

https://codereview.appspot.com/9671043/diff/19002/google-api-go-generator/gen.go#newcode1155
google-api-go-generator/gen.go:1155: pn("req.URL.Opaque = \"//\" +
req.URL.Host + req.URL.Path\n")
ditto

https://codereview.appspot.com/9671043/

--

---
You received this message because you are subscribed to the Google Groups "golang-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [email protected].
For more options, visit https://groups.google.com/groups/opt_out.

reply | permalink

Campoy

at May 23, 2013 at 12:53 am

⇧

https://codereview.appspot.com/9671043/diff/19002/google-api-go-generator/gen.go
File google-api-go-generator/gen.go (right):

https://codereview.appspot.com/9671043/diff/19002/google-api-go-generator/gen.go#newcode1152
google-api-go-generator/gen.go:1152: pn("req.URL.Path =
strings.Replace(req.URL.Path, \"{%s}\", %s, 1)\n", arg.apiname,
arg.cleanExpr("c."))

On 2013/05/23 00:45:36, adg wrote:
AGAIN, please use `backtics` to avoid the needless quoting

Done
I was keeping it that way just to be consistent with the code around it.

https://codereview.appspot.com/9671043/diff/19002/google-api-go-generator/gen.go#newcode1155
google-api-go-generator/gen.go:1155: pn("req.URL.Opaque = \"//\" +
req.URL.Host + req.URL.Path\n")

On 2013/05/23 00:45:36, adg wrote:
ditto

Done.

https://codereview.appspot.com/9671043/

--

---
You received this message because you are subscribed to the Google Groups "golang-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [email protected].
For more options, visit https://groups.google.com/groups/opt_out.

reply | permalink

Adg

at May 23, 2013 at 2:31 am

⇧

This looks ok to me now, but wait for brad.

https://codereview.appspot.com/9671043/diff/19002/google-api-go-generator/gen.go
File google-api-go-generator/gen.go (right):

https://codereview.appspot.com/9671043/diff/19002/google-api-go-generator/gen.go#newcode1152
google-api-go-generator/gen.go:1152: pn("req.URL.Path =
strings.Replace(req.URL.Path, \"{%s}\", %s, 1)\n", arg.apiname,
arg.cleanExpr("c."))

On 2013/05/23 00:53:23, gocampoy wrote:

On 2013/05/23 00:45:36, adg wrote:
AGAIN, please use `backtics` to avoid the needless quoting

Done
I was keeping it that way just to be consistent with the code around

it.

Look at the surrounding code. It uses backticks everywhere there are
quotes inside the strings.

https://codereview.appspot.com/9671043/

--

---
You received this message because you are subscribed to the Google Groups "golang-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [email protected].
For more options, visit https://groups.google.com/groups/opt_out.

reply | permalink

Francesc Campoy Flores

at May 23, 2013 at 5:35 pm

⇧

Gentle ping for Brad

On Wed, May 22, 2013 at 7:31 PM, wrote:

This looks ok to me now, but wait for brad.

https://codereview.appspot.**com/9671043/diff/19002/google-**
api-go-generator/gen.go<https://codereview.appspot.com/9671043/diff/19002/google-api-go-generator/gen.go>
File google-api-go-generator/gen.go (right):

https://codereview.appspot.**com/9671043/diff/19002/google-**
api-go-generator/gen.go#**newcode1152<https://codereview.appspot.com/9671043/diff/19002/google-api-go-generator/gen.go#newcode1152>
google-api-go-generator/gen.**go:1152: pn("req.URL.Path =
strings.Replace(req.URL.Path, \"{%s}\", %s, 1)\n", arg.apiname,
arg.cleanExpr("c."))

On 2013/05/23 00:53:23, gocampoy wrote:

On 2013/05/23 00:45:36, adg wrote:
AGAIN, please use `backtics` to avoid the needless quoting

Done

I was keeping it that way just to be consistent with the code around

it.

Look at the surrounding code. It uses backticks everywhere there are
quotes inside the strings.

https://codereview.appspot.**com/9671043/<https://codereview.appspot.com/9671043/>

--
--
Francesc

--

---
You received this message because you are subscribed to the Google Groups "golang-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [email protected].
For more options, visit https://groups.google.com/groups/opt_out.

reply | permalink

Bradfitz

at May 23, 2013 at 9:15 pm

⇧

LGTM

I think.

Be sure to ask people to test it go golang-nuts after you push out the
generated changes.

I'd also like to see some sample code / tests checked in at some point.

https://codereview.appspot.com/9671043/

--

---
You received this message because you are subscribed to the Google Groups "golang-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [email protected].
For more options, visit https://groups.google.com/groups/opt_out.

reply | permalink

Campoy

at May 23, 2013 at 9:18 pm

⇧

*** Submitted as
https://code.google.com/p/google-api-go-client/source/detail?r=49979a5f1445
***

google-api-go-client: avoid reencoding parameters in the URL path

Some APIs, such as cloud storage, need to use values that are being
now removed.
'%2F' is replaced by '/' url.Parse, which is called by http.NewRequest.

R=bradfitz, adg
CC=golang-dev
https://codereview.appspot.com/9671043

https://codereview.appspot.com/9671043/

--

---
You received this message because you are subscribed to the Google Groups "golang-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [email protected].
For more options, visit https://groups.google.com/groups/opt_out.

reply | permalink

group	golang-dev @
categories	go
posted	May 22, '13 at 11:22p
active	May 23, '13 at 9:18p
posts	15
users	3
website	golang.org

responses	expanded	Hotkey: s
font	variable	Hotkey: f
user style	avatars	Hotkey: a

[golang-dev] Re: code review 9671043: google-api-go-client: avoid reencoding parameters in th... (issue 9671043)

Search Discussions

Related Discussions

3 users in discussion