oauth2: Introduce an option function type

- Reduce the duplicate code by merging the flows and determining the flow type by looking at the provided options. - Options as a function type allows us to validate an individual an option in its scope and makes it easier to compose the built-in options with the third-party ones.

[adg1, 2014-11-06 22:52:42]

https://codereview.appspot.com/165550043/diff/1/google/example_test.go File google/example_test.go (right): https://codereview.appspot.com/165550043/diff/1/google/example_test.go#newcode32 google/example_test.go:32: "https://www.googleapis.com/auth/blogger"), prefer ", ) https://codereview.appspot.com/165550043/diff/1/google/example_test.go#newcod... google/example_test.go:131: google.ComputeEngineAccount(""), put an argument here? what is this? https://codereview.appspot.com/165550043/diff/1/oauth2.go File oauth2.go (right): https://codereview.appspot.com/165550043/diff/1/oauth2.go#newcode88 oauth2.go:88: opts *Options needn't be a pointer https://codereview.appspot.com/165550043/diff/1/oauth2.go#newcode92 oauth2.go:92: f := &Flow{ var f Flow https://codereview.appspot.com/165550043/diff/1/oauth2.go#newcode96 oauth2.go:96: if err := opt(f.opts); err != nil { &f.opts https://codereview.appspot.com/165550043/diff/1/oauth2.go#newcode104 oauth2.go:104: if f.opts.AUD != nil { switch statement? https://codereview.appspot.com/165550043/diff/1/oauth2.go#newcode106 oauth2.go:106: // Assert required JWT params. are these todos?

[adg1, 2014-11-06 22:56:30]

https://codereview.appspot.com/165550043/diff/1/oauth2.go File oauth2.go (right): https://codereview.appspot.com/165550043/diff/1/oauth2.go#newcode227 oauth2.go:227: func (o *Options) retrieveToken(v url.Values) (*Token, error) { I think this should just be a function, not a method on *Options

[jbd, 2014-11-06 23:23:52]

https://codereview.appspot.com/165550043/diff/1/google/example_test.go File google/example_test.go (right): https://codereview.appspot.com/165550043/diff/1/google/example_test.go#newcode32 google/example_test.go:32: "https://www.googleapis.com/auth/blogger"), On 2014/11/06 22:52:41, adg1 wrote: > prefer > ", > ) Done. https://codereview.appspot.com/165550043/diff/1/google/example_test.go#newcod... google/example_test.go:131: google.ComputeEngineAccount(""), On 2014/11/06 22:52:41, adg1 wrote: > put an argument here? what is this? Done. https://codereview.appspot.com/165550043/diff/1/oauth2.go File oauth2.go (right): https://codereview.appspot.com/165550043/diff/1/oauth2.go#newcode88 oauth2.go:88: opts *Options On 2014/11/06 22:52:42, adg1 wrote: > needn't be a pointer Done. https://codereview.appspot.com/165550043/diff/1/oauth2.go#newcode92 oauth2.go:92: f := &Flow{ On 2014/11/06 22:52:41, adg1 wrote: > var f Flow Do you want me to return a Flow instead of a *Flow? https://codereview.appspot.com/165550043/diff/1/oauth2.go#newcode96 oauth2.go:96: if err := opt(f.opts); err != nil { On 2014/11/06 22:52:42, adg1 wrote: > &f.opts Done. https://codereview.appspot.com/165550043/diff/1/oauth2.go#newcode104 oauth2.go:104: if f.opts.AUD != nil { On 2014/11/06 22:52:42, adg1 wrote: > switch statement? There are far too many variables involved in the condition statements. Using switch won't make it more readable. https://codereview.appspot.com/165550043/diff/1/oauth2.go#newcode106 oauth2.go:106: // Assert required JWT params. On 2014/11/06 22:52:41, adg1 wrote: > are these todos? Done. https://codereview.appspot.com/165550043/diff/1/oauth2.go#newcode227 oauth2.go:227: func (o *Options) retrieveToken(v url.Values) (*Token, error) { On 2014/11/06 22:56:30, adg1 wrote: > I think this should just be a function, not a method on *Options Done.

[adg1, 2014-11-06 23:29:51]

https://codereview.appspot.com/165550043/diff/1/oauth2.go File oauth2.go (right): https://codereview.appspot.com/165550043/diff/1/oauth2.go#newcode92 oauth2.go:92: f := &Flow{ On 2014/11/06 23:23:52, jbd wrote: > On 2014/11/06 22:52:41, adg1 wrote: > > var f Flow > > Do you want me to return a Flow instead of a *Flow? no; return &f https://codereview.appspot.com/165550043/diff/1/oauth2.go#newcode104 oauth2.go:104: if f.opts.AUD != nil { On 2014/11/06 23:23:52, jbd wrote: > On 2014/11/06 22:52:42, adg1 wrote: > > switch statement? > > There are far too many variables involved in the condition statements. Using > switch won't make it more readable. to me, the switch indicates that there are a few distinct paths for the code to take. almost always clearer than if/else imo

[jbd, 2014-11-07 00:35:03]

> to me, the switch indicates that there are a few distinct paths for the code to take. almost always clearer than if/else imo Yes, you are right. It looks significantly cuter now.

[adg1, 2014-11-07 00:39:20]

^_^ On Fri Nov 07 2014 at 11:35:03 AM <jbd@google.com> wrote: > > to me, the switch indicates that there are a few distinct paths for > the code to > take. almost always clearer than if/else imo > > Yes, you are right. It looks significantly cuter now. > > https://codereview.appspot.com/165550043/ > 

[jbd, 2014-11-07 02:59:16]

Btw, if the Flow is type Flow struct {	opts Options } Maybe Options is the Flow.

[adg1, 2014-11-07 03:02:07]

Well this protects the options from being manipulated after the object is created. On Fri, 7 Nov 2014 at 13:59, <jbd@google.com> wrote: > Btw, if the Flow is > > type Flow struct { > opts Options > } > > Maybe Options is the Flow. > > https://codereview.appspot.com/165550043/ > 

[jbd, 2014-11-07 03:19:14]

> Well this protects the options from being manipulated after the object is > created. Yes, I didn't mentioned it since it's obvious that options should be immutable once a flow is constructed. Keeping it as Flow, open to suggestions to rename it.

[jbd, 2014-11-07 03:56:44]

PTAL

[jbd, 2014-11-10 03:13:54]

Can I have a LGTM?

[adg1, 2014-11-10 03:16:53]

Yeah, all the sub-repo stuff jumped in the way this morning but I'm reviewing this now. On Mon Nov 10 2014 at 2:13:54 PM <jbd@google.com> wrote: > Can I have a LGTM? > > > https://codereview.appspot.com/165550043/ > 

[adg1, 2014-11-10 03:24:14]

LGTM

[jbd, 2014-11-10 03:54:18]

Submitted as 0cf6f9b144f92af7a901094e39b22c310f4ea2cd. BREAK ALL THE BUILDS!!!!!