code review 5450050: gofix: add time+fileinfo fix

gofix: add time+fileinfo fix

[rsc, 2011-11-30 04:07:22]

Hello golang-dev@googlegroups.com,

I'd like you to review this change to
https://go.googlecode.com/hg/

[adg, 2011-11-30 04:56:18]

http://codereview.appspot.com/5450050/diff/2001/src/cmd/gofix/timefileinfo_te...
File src/cmd/gofix/timefileinfo_test.go (right):

http://codereview.appspot.com/5450050/diff/2001/src/cmd/gofix/timefileinfo_te...
src/cmd/gofix/timefileinfo_test.go:114: _ = time.Now()
shouldn't these be
  _ = time.Now().Unix()
  _ = time.Now().UnixNano()
?

[rsc, 2011-11-30 14:30:43]

http://codereview.appspot.com/5450050/diff/2001/src/cmd/gofix/timefileinfo_te...
File src/cmd/gofix/timefileinfo_test.go (right):

http://codereview.appspot.com/5450050/diff/2001/src/cmd/gofix/timefileinfo_te...
src/cmd/gofix/timefileinfo_test.go:114: _ = time.Now()
On 2011/11/30 04:56:18, adg wrote:
> shouldn't these be
>   _ = time.Now().Unix()
>   _ = time.Now().UnixNano()
> ?

If you were trying to keep existing code compiling, yes.
But I am trying instead to convert expressions to what
people should be writing using the new API.  There will
be type errors for people to fix.

Gofix is about helping to convert code to use the new API,
not preserving the exact semantics of existing code.

[rog, 2011-11-30 15:29:53]

http://codereview.appspot.com/5450050/diff/2001/src/cmd/gofix/timefileinfo_te...
File src/cmd/gofix/timefileinfo_test.go (right):

http://codereview.appspot.com/5450050/diff/2001/src/cmd/gofix/timefileinfo_te...
src/cmd/gofix/timefileinfo_test.go:114: _ = time.Now()
On 2011/11/30 14:30:43, rsc wrote:
> On 2011/11/30 04:56:18, adg wrote:
> > shouldn't these be
> >   _ = time.Now().Unix()
> >   _ = time.Now().UnixNano()
> > ?
> 
> If you were trying to keep existing code compiling, yes.
> But I am trying instead to convert expressions to what
> people should be writing using the new API.  There will
> be type errors for people to fix.
> 
> Gofix is about helping to convert code to use the new API,
> not preserving the exact semantics of existing code.

the problem i have with this is that on looking at the gofixed code, it's not
obvious what units the original file was expecting - it could be awkward and
error prone to match the gofixed Nows with the original Seconds or Nanoseconds
calls.
i think i'd convert Nanoseconds (the more common call) to Now, but Seconds to
Now().Unix.

[rsc, 2011-11-30 15:44:20]

On Wed, Nov 30, 2011 at 10:29,  <rogpeppe@gmail.com> wrote:
> the problem i have with this is that on looking at the gofixed code,
> it's not obvious what units the original file was expecting - it could
> be awkward and error prone to match the gofixed Nows with the original
> Seconds or Nanoseconds calls.
> i think i'd convert Nanoseconds (the more common call) to Now, but
> Seconds to Now().Unix.

I expect people to check the diffs after running gofix.
There were many examples of code in the tree where
the correct rewrite for time.Seconds() was time.Now().
If we rewrite to time.Now().Unix() then we will end up
with clunky code that compiles instead of clean code
that doesn't compile.  I am going for the latter, so
that the developer can do the rest (which usually involves
thought about data types).

Russ

[r, 2011-11-30 17:18:36]

this module will require better public explanation that most of the others.
there's a belief that gofix fixes, but it only tries to fix. in this one, it
fails much more often than the others, and the changes are more sweeping. we
will need citable documentation

http://codereview.appspot.com/5450050/diff/2001/src/cmd/gofix/timefileinfo.go
File src/cmd/gofix/timefileinfo.go (right):

http://codereview.appspot.com/5450050/diff/2001/src/cmd/gofix/timefileinfo.go...
src/cmd/gofix/timefileinfo.go:72: called := make(map[interface{}]bool)
this variable's purpose 's mysterious enough to deserve a comment. it took me a
couple of readthroughs to convince myself what it was doing and another that it
would work. also the doc comment doesn't mention explicitly enough the test it
does.

http://codereview.appspot.com/5450050/diff/2001/src/cmd/gofix/timefileinfo.go...
src/cmd/gofix/timefileinfo.go:80: return
this is fine but early out would obviously speed things up. not a
recommendation, just a suggestion: use panic/recover to avoid the full walk, if
only to have the pattern available in this tool.

http://codereview.appspot.com/5450050/diff/2001/src/cmd/gofix/timefileinfo.go...
src/cmd/gofix/timefileinfo.go:162: // The types might not work anymore, but the
compiler will catch that.
s/anymore/any more/

but really you mean

s/any more/after this edit/

[rsc, 2011-11-30 17:20:25]

On Wed, Nov 30, 2011 at 12:18,  <r@golang.org> wrote:
> this module will require better public explanation that most of the
> others. there's a belief that gofix fixes, but it only tries to fix. in
> this one, it fails much more often than the others, and the changes are
> more sweeping. we will need citable documentation

more generally, we need documentation like this for all
the major changes in Go 1.

[rsc, 2011-11-30 17:28:42]

PTAL

I will draft some text for Andrew to include in the weekly notes.

[r, 2011-11-30 17:35:17]

LGTM but fix comment

http://codereview.appspot.com/5450050/diff/1005/src/cmd/gofix/timefileinfo.go
File src/cmd/gofix/timefileinfo.go (right):

http://codereview.appspot.com/5450050/diff/1005/src/cmd/gofix/timefileinfo.go...
src/cmd/gofix/timefileinfo.go:68: //	an attempt to call
an attempt to call what?

http://codereview.appspot.com/5450050/diff/1005/src/cmd/gofix/timefileinfo.go...
src/cmd/gofix/timefileinfo.go:73: // called records the expression that appear
as
s/expression/&s/

[cw, 2011-11-30 19:25:20]

LGTM

Having just updated a few things, I'd like to see a draft of the release notes /
docs explaining how to update things.

I have cases where I did a quick pass to make things build and work (ie.
time.Now.UnixNano() with math) but cleaner options exist now and having those
explained would be useful).

[rsc, 2011-12-01 18:59:59]

*** Submitted as http://code.google.com/p/go/source/detail?r=7299b8a26342 ***

gofix: add time+fileinfo fix

R=adg, rogpeppe, r, cw
CC=golang-dev
http://codereview.appspot.com/5450050