code review 5629060: goauth2: update for Go 1

goauth2: update for Go 1

[adg, 2012-02-05 12:56:54]

Hello proppy, M-A (cc: golang-dev@googlegroups.com),

I'd like you to review this change to
https://code.google.com/p/goauth2

[M-A, 2012-02-05 13:21:21]

lgtm

Do you mind incorporating the example change there:
http://codereview.appspot.com/5632043/ ?

It was slightly disheartening to look at an example about buzz. The profile
example gives small bits of information that is simpler to understand IMHO.

http://codereview.appspot.com/5629060/diff/6001/oauth/oauth.go
File oauth/oauth.go (right):

http://codereview.appspot.com/5629060/diff/6001/oauth/oauth.go#newcode76
oauth/oauth.go:76: if t.Expiry == nil {
note: I didn't feel it easier to understand to have it on 3 lines, it happily
fits on one.

http://codereview.appspot.com/5629060/diff/6001/oauth/oauth.go#newcode221
oauth/oauth.go:221: tok.Expiry = &e
Why use a named variable?
tok.Expiry = &time.Now().Add(b.ExpiresIn * time.Second)
would be fine?

[adg, 2012-02-05 13:24:50]

I'd prefer to take your example change in a separate CL, if you don't mind.

http://codereview.appspot.com/5629060/diff/6001/oauth/oauth.go
File oauth/oauth.go (right):

http://codereview.appspot.com/5629060/diff/6001/oauth/oauth.go#newcode76
oauth/oauth.go:76: if t.Expiry == nil {
On 2012/02/05 13:21:22, M-A wrote:
> note: I didn't feel it easier to understand to have it on 3 lines, it happily
> fits on one.

Why use one line when 3 lines will do? :-)

http://codereview.appspot.com/5629060/diff/6001/oauth/oauth.go#newcode221
oauth/oauth.go:221: tok.Expiry = &e
On 2012/02/05 13:21:22, M-A wrote:
> Why use a named variable?
> tok.Expiry = &time.Now().Add(b.ExpiresIn * time.Second)
> would be fine?

No, that won't work. The return value of a function is not addressable.
(http://weekly.golang.org/doc/go_spec.html#Address_operators)

$ cat foo.go 
package main

import "time"

func main() {
	t := &time.Now()
	_ = t
}
$ go run foo.go
# _/foo
/Users/nf/foo.go:6: cannot take the address of time.Now()

[ality, 2012-02-05 14:06:55]

adg@golang.org once said:
> >Why use a named variable?
> >tok.Expiry = &time.Now().Add(b.ExpiresIn * time.Second)
> >would be fine?
> 
> No, that won't work. The return value of a function is not addressable.
> (http://weekly.golang.org/doc/go_spec.html#Address_operators)

You could make Expiry a value instead of a pointer.
Then it would be:

	tok.Expiry = time.Now().Add(b.ExpiresIn * time.Second)

And instead of checking for nil you would use the IsZero method.

Cheers,
  Anthony

[M-A, 2012-02-05 14:13:30]

On 2012/02/05 13:24:50, adg wrote:
> I'd prefer to take your example change in a separate CL, if you don't mind.

np. I updated 5632043 in case.

Thanks for the bits about return value address, I'm still learning.


On 2012/02/05 14:06:55, ality wrote:
> mailto:adg@golang.org once said:
> > >Why use a named variable?
> > >tok.Expiry = &time.Now().Add(b.ExpiresIn * time.Second)
> > >would be fine?
> > 
> > No, that won't work. The return value of a function is not addressable.
> > (http://weekly.golang.org/doc/go_spec.html#Address_operators)
> 
> You could make Expiry a value instead of a pointer.
> Then it would be:
> 
> 	tok.Expiry = time.Now().Add(b.ExpiresIn * time.Second)
> 
> And instead of checking for nil you would use the IsZero method.

I tend to agree with Anthony. Not using a pointer here reduces the likelihood of
messing up.

Note that my original CL I thought about using time.Time but didn't because I
wanted to keep the CL as simple as possible. :)

[M-A, 2012-02-05 16:29:02]

http://codereview.appspot.com/5629060/diff/6001/oauth/oauth.go
File oauth/oauth.go (right):

http://codereview.appspot.com/5629060/diff/6001/oauth/oauth.go#newcode187
oauth/oauth.go:187: return errors.New("no exisiting Token")
existing

[adg, 2012-02-05 22:33:07]

On 6 February 2012 01:06, Anthony Martin <ality@pbrane.org> wrote:
> instead of checking for nil you would use the IsZero method.

I didn't notice IsZero. Thanks for the suggestion. I've made Expiry a
time.Time value now.

Andrew

[adg, 2012-02-05 22:33:12]

http://codereview.appspot.com/5629060/diff/6001/oauth/oauth.go
File oauth/oauth.go (right):

http://codereview.appspot.com/5629060/diff/6001/oauth/oauth.go#newcode187
oauth/oauth.go:187: return errors.New("no exisiting Token")
On 2012/02/05 16:29:02, M-A wrote:
> existing

Done.

[adg, 2012-02-05 23:40:52]

*** Submitted as 4ee7c273e92e ***

goauth2: update for Go 1

R=proppy, maruel, ality
CC=golang-dev
http://codereview.appspot.com/5629060

[M-A, 2012-02-06 00:21:12]

http://codereview.appspot.com/5629060/diff/7002/oauth/oauth.go
File oauth/oauth.go (right):

http://codereview.appspot.com/5629060/diff/7002/oauth/oauth.go#newcode218
oauth/oauth.go:218: tok.Expiry = time.Unix(0, 0)
Sadly, this won't trigger IsZero() at line 76.

"IsZero reports whether t represents the zero time instant, January 1, year 1,
00:00:00 UTC."

[ality, 2012-02-06 00:34:27]

maruel@chromium.org once said:
> http://codereview.appspot.com/5629060/diff/7002/oauth/oauth.go#newcode218
> oauth/oauth.go:218: tok.Expiry = time.Unix(0, 0)
> Sadly, this won't trigger IsZero() at line 76.

True.  The test needs to be:

if b.ExpiresIn > 0 {
	tok.Expiry = time.Now().Add(b.ExpiresIn * time.Second)
}

[proppy, 2012-02-06 11:23:37]

http://codereview.appspot.com/5629060/diff/7002/oauth/oauth.go
File oauth/oauth.go (right):

http://codereview.appspot.com/5629060/diff/7002/oauth/oauth.go#newcode218
oauth/oauth.go:218: tok.Expiry = time.Unix(0, 0)
On 2012/02/06 00:21:12, M-A wrote:
> Sadly, this won't trigger IsZero() at line 76.
> 
> "IsZero reports whether t represents the zero time instant, January 1, year 1,
> 00:00:00 UTC."

time.Time{}.IsZero() should work.