oauth2: Removing the inconsistent and duplicate features, better naming

oauth2: Removing the inconsistent and duplicate features, better naming

- Removed Flow, Flow is nothing but Options.
- Renamed Cacher to Storer.
- Removed the setter from the Transport. Store should do the initial set.
   Getter is not removed, because extra fields are available through
   Transport.Token.Extra() and it's not pleasant to implement a custom Storer
   to read such values.
- Base RoundTripper option is removed, depend on the provided http.Client.

[jbd, 2014-11-25 01:23:16]

I haven't thought about how to better abstract the flows yet, but these are some
immediate improvements we can make.

Sending out the CL for you to take a look, the subpackages and the tests require
work.

[bradfitz, 2014-11-25 01:41:18]

https://codereview.appspot.com/178130044/diff/20001/oauth2.go
File oauth2.go (right):

https://codereview.appspot.com/178130044/diff/20001/oauth2.go#newcode26
oauth2.go:26: type Storer interface {
If you name it Fooer, I would expect a Foo method.

I like "TokenStore". It says what it operates on too.

https://codereview.appspot.com/178130044/diff/20001/oauth2.go#newcode32
oauth2.go:32: Read() (*Token, error)
I know nobody will confuse it for an io.Reader, but what about ReadToken and
StoreToken? I like Token in the verbs because then call sites like:

      cache.ReadToken()

... are more obvious about what it returns, than:

      cache.Read()

https://codereview.appspot.com/178130044/diff/20001/oauth2.go#newcode96
oauth2.go:96: // New initiates a new flow. It determines the type of the OAuth
2.0
drop the word flow?

https://codereview.appspot.com/178130044/diff/20001/oauth2.go#newcode101
oauth2.go:101: for _, opt := range option {
call this opt "fn" ?

https://codereview.appspot.com/178130044/diff/20001/oauth2.go#newcode106
oauth2.go:106: switch {
yeah we should fix the constructor story later. first time seeing this code.

https://codereview.appspot.com/178130044/diff/20001/oauth2.go#newcode272
oauth2.go:272: Store Storer
document this while you're here. is it required or optional?

https://codereview.appspot.com/178130044/diff/20001/transport.go
File transport.go (right):

https://codereview.appspot.com/178130044/diff/20001/transport.go#newcode120
transport.go:120: func (t *Transport) Token() *Token {
why? what if it changes while the caller accesses it?

[jbd, 2014-11-25 03:08:42]

PTAL

https://codereview.appspot.com/178130044/diff/20001/oauth2.go
File oauth2.go (right):

https://codereview.appspot.com/178130044/diff/20001/oauth2.go#newcode26
oauth2.go:26: type Storer interface {
On 2014/11/25 01:41:17, bradfitz wrote:
> If you name it Fooer, I would expect a Foo method.
> 
> I like "TokenStore". It says what it operates on too.

We should find a better name that's suitable to an -er prefix.

Done.

https://codereview.appspot.com/178130044/diff/20001/oauth2.go#newcode32
oauth2.go:32: Read() (*Token, error)
On 2014/11/25 01:41:17, bradfitz wrote:
> I know nobody will confuse it for an io.Reader, but what about ReadToken and
> StoreToken? I like Token in the verbs because then call sites like:
> 
>       cache.ReadToken()
> 
> ... are more obvious about what it returns, than:
> 
>       cache.Read()

Done.

But, I don't like the way we put a "token" in every possible name.

https://codereview.appspot.com/178130044/diff/20001/oauth2.go#newcode96
oauth2.go:96: // New initiates a new flow. It determines the type of the OAuth
2.0
On 2014/11/25 01:41:17, bradfitz wrote:
> drop the word flow?

Done.

https://codereview.appspot.com/178130044/diff/20001/oauth2.go#newcode101
oauth2.go:101: for _, opt := range option {
On 2014/11/25 01:41:17, bradfitz wrote:
> call this opt "fn" ?

Done.

https://codereview.appspot.com/178130044/diff/20001/oauth2.go#newcode106
oauth2.go:106: switch {
On 2014/11/25 01:41:17, bradfitz wrote:
> yeah we should fix the constructor story later. first time seeing this code.

Acknowledged.

https://codereview.appspot.com/178130044/diff/20001/oauth2.go#newcode272
oauth2.go:272: Store Storer
On 2014/11/25 01:41:17, bradfitz wrote:
> document this while you're here. is it required or optional?

Done.

https://codereview.appspot.com/178130044/diff/20001/transport.go
File transport.go (right):

https://codereview.appspot.com/178130044/diff/20001/transport.go#newcode120
transport.go:120: func (t *Transport) Token() *Token {
On 2014/11/25 01:41:17, bradfitz wrote:
> why? what if it changes while the caller accesses it?


Do you mean the case

tok := tr.Token()
make some request that invokes token refreshing with tr

and what happens to the tok?

[adg1, 2014-11-25 05:43:24]

LGTM

[jbd, 2014-11-25 06:44:01]

Updated the tests.

[jbd, 2014-11-25 22:51:19]

Submitted as b8463885646621ad3debe22376c60031788d9f5e.