code review 150890043: google-api-go-client: Implement partial requests by add...

google-api-go-client: Implement partial requests by adding Fields option.
        Demonstration of partial requests added to calendar example.
        As a result, only the calender:v3 API has been updated.
        All remaining APIs will be updated in a separate CL.

[gmlewis1, 2014-09-24 05:14:03]

Hello bradfitz@golang.org (cc: golang-codereviews@googlegroups.com),

I'd like you to review this change to
https://code.google.com/p/google-api-go-client

[bradfitz, 2014-09-24 17:05:35]

https://codereview.appspot.com/150890043/diff/40001/examples/calendar.go
File examples/calendar.go (right):

https://codereview.appspot.com/150890043/diff/40001/examples/calendar.go#newc...
examples/calendar.go:53: listRes, err :=
svc.CalendarList.List().Fields("items/id").Do()
whoa, I would've never guessed it might have a slash in it

https://codereview.appspot.com/150890043/diff/40001/examples/calendar.go#newc...
examples/calendar.go:63: res, err :=
svc.Events.List(id).Fields("items(updated,summary)", "summary",
"nextPageToken").Do()
or this syntax.

https://codereview.appspot.com/150890043/diff/40001/google-api-go-generator/g...
File google-api-go-generator/gen.go (right):

https://codereview.appspot.com/150890043/diff/40001/google-api-go-generator/g...
google-api-go-generator/gen.go:1232: pn("func (c *%s) Fields(s ...string) *%s
{", callName, callName)
where will users find the list of 's'?  Perhaps we should instead make this be
"...googleapi.Field" and do:

package googleapi

// A Field names a field to be retrieved with a partial response.
// See https://developers.google.com/gdata/docs/2.0/basics#PartialResponse
type Field string

And then in auto-generated APIs, provide methods/tables to lookup or generate
Fields strings, so things are optionally type-checked. But because the
underlying type is still string, people can use constant literals the same way
as in this CL.

Perhaps we just do the type Field in this CL, and do the builders/methods to
return Field values later.

[gmlewis1, 2014-09-24 17:57:54]

PTAL

https://codereview.appspot.com/150890043/diff/40001/examples/calendar.go
File examples/calendar.go (right):

https://codereview.appspot.com/150890043/diff/40001/examples/calendar.go#newc...
examples/calendar.go:53: listRes, err :=
svc.CalendarList.List().Fields("items/id").Do()
On 2014/09/24 17:05:34, bradfitz wrote:
> whoa, I would've never guessed it might have a slash in it

Right!  I wanted to include some shocking examples here and below.  :-)
Getting the "nextPageToken" example to work was fun... but that is how it seems
to work... camelCase starting with a lower-case letter.

https://codereview.appspot.com/150890043/diff/40001/examples/calendar.go#newc...
examples/calendar.go:63: res, err :=
svc.Events.List(id).Fields("items(updated,summary)", "summary",
"nextPageToken").Do()
On 2014/09/24 17:05:34, bradfitz wrote:
> or this syntax.

Acknowledged.

https://codereview.appspot.com/150890043/diff/40001/google-api-go-generator/g...
File google-api-go-generator/gen.go (right):

https://codereview.appspot.com/150890043/diff/40001/google-api-go-generator/g...
google-api-go-generator/gen.go:1232: pn("func (c *%s) Fields(s ...string) *%s
{", callName, callName)
On 2014/09/24 17:05:34, bradfitz wrote:
> where will users find the list of 's'?  Perhaps we should instead make this be
> "...googleapi.Field" and do:
> 
> package googleapi
> 
> // A Field names a field to be retrieved with a partial response.
> // See https://developers.google.com/gdata/docs/2.0/basics#PartialResponse
> type Field string
> 
> And then in auto-generated APIs, provide methods/tables to lookup or generate
> Fields strings, so things are optionally type-checked. But because the
> underlying type is still string, people can use constant literals the same way
> as in this CL.
> 
> Perhaps we just do the type Field in this CL, and do the builders/methods to
> return Field values later.

OK, SGTM.

[bradfitz, 2014-09-24 18:21:32]

LGTM

Looks great, thanks!

https://codereview.appspot.com/150890043/diff/60001/google-api-go-generator/g...
File google-api-go-generator/gen.go (right):

https://codereview.appspot.com/150890043/diff/60001/google-api-go-generator/g...
google-api-go-generator/gen.go:1268: opts = append(opts, &Param{name: "fields"})
do you have to reserve this name earlier in the namePool so a parameter named
"fields" doesn't conflict with this one?

It's unlikely, but we've been hit by similar bugs before.

But I guess type Method doesn't have a namePool like API does.

Not a big deal, but if you see an easy way/place to reserve it, go for it.

https://codereview.appspot.com/150890043/diff/60001/googleapi/googleapi.go
File googleapi/googleapi.go (right):

https://codereview.appspot.com/150890043/diff/60001/googleapi/googleapi.go#ne...
googleapi/googleapi.go:370: // See
https://developers.google.com/gdata/docs/2.0/basics#PartialResponse
want to list some examples here too, from simple syntax up to crazy syntaxes
like in your example file? Use the right formatting so it looks nice in godoc.

[gmlewis1, 2014-09-24 21:18:14]

PTAL

https://codereview.appspot.com/150890043/diff/60001/google-api-go-generator/g...
File google-api-go-generator/gen.go (right):

https://codereview.appspot.com/150890043/diff/60001/google-api-go-generator/g...
google-api-go-generator/gen.go:1268: opts = append(opts, &Param{name: "fields"})
On 2014/09/24 18:21:31, bradfitz wrote:
> do you have to reserve this name earlier in the namePool so a parameter named
> "fields" doesn't conflict with this one?
> 
> It's unlikely, but we've been hit by similar bugs before.
> 
> But I guess type Method doesn't have a namePool like API does.
> 
> Not a big deal, but if you see an easy way/place to reserve it, go for it.

Hmmm... I don't see any easy way or place to reserve it.
I'm thinking that if anyone adds a "fields" parameter to their API, that Apiary
will
hopefully notify them of the conflict since this would mess up their JSON API
and partial responses.
So I'll leave it for now.

https://codereview.appspot.com/150890043/diff/60001/googleapi/googleapi.go
File googleapi/googleapi.go (right):

https://codereview.appspot.com/150890043/diff/60001/googleapi/googleapi.go#ne...
googleapi/googleapi.go:370: // See
https://developers.google.com/gdata/docs/2.0/basics#PartialResponse
On 2014/09/24 18:21:32, bradfitz wrote:
> want to list some examples here too, from simple syntax up to crazy syntaxes
> like in your example file? Use the right formatting so it looks nice in godoc.

I wrote some examples and checked the results using "godoc -http 6060" and they
look good to me, but maybe you would take a look to see if this is what you had
in mind if you have an opportunity.  Thanks!

[bradfitz, 2014-09-24 22:57:25]

LGTM

https://codereview.appspot.com/150890043/diff/80001/googleapi/googleapi.go
File googleapi/googleapi.go (right):

https://codereview.appspot.com/150890043/diff/80001/googleapi/googleapi.go#ne...
googleapi/googleapi.go:378: // For example, if your response has a
"NextPageToken" and a slice if "Items" with "Id"s,
slice of

https://codereview.appspot.com/150890043/diff/80001/googleapi/googleapi.go#ne...
googleapi/googleapi.go:378: // For example, if your response has a
"NextPageToken" and a slice if "Items" with "Id"s,
with "Id" fields

https://codereview.appspot.com/150890043/diff/80001/googleapi/googleapi.go#ne...
googleapi/googleapi.go:379: // you could request just these fields like this:
just those

https://codereview.appspot.com/150890043/diff/80001/googleapi/googleapi.go#ne...
googleapi/googleapi.go:390: // Another great way to find field names is through
the Google API explorer:
drop the word great. sounds like advertising.

[gmlewis1, 2014-09-24 23:10:07]

Thank you, Brad!

https://codereview.appspot.com/150890043/diff/80001/googleapi/googleapi.go
File googleapi/googleapi.go (right):

https://codereview.appspot.com/150890043/diff/80001/googleapi/googleapi.go#ne...
googleapi/googleapi.go:378: // For example, if your response has a
"NextPageToken" and a slice if "Items" with "Id"s,
On 2014/09/24 22:57:24, bradfitz wrote:
> slice of

Done.

https://codereview.appspot.com/150890043/diff/80001/googleapi/googleapi.go#ne...
googleapi/googleapi.go:378: // For example, if your response has a
"NextPageToken" and a slice if "Items" with "Id"s,
On 2014/09/24 22:57:24, bradfitz wrote:
> with "Id" fields

Done.

https://codereview.appspot.com/150890043/diff/80001/googleapi/googleapi.go#ne...
googleapi/googleapi.go:379: // you could request just these fields like this:
On 2014/09/24 22:57:24, bradfitz wrote:
> just those

Done.

https://codereview.appspot.com/150890043/diff/80001/googleapi/googleapi.go#ne...
googleapi/googleapi.go:390: // Another great way to find field names is through
the Google API explorer:
On 2014/09/24 22:57:24, bradfitz wrote:
> drop the word great. sounds like advertising.

Done.

[gmlewis1, 2014-09-24 23:11:31]

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

google-api-go-client: Implement partial requests by adding Fields option.
        Demonstration of partial requests added to calendar example.
        As a result, only the calender:v3 API has been updated.
        All remaining APIs will be updated in a separate CL.

R=bradfitz
CC=golang-codereviews
https://codereview.appspot.com/150890043