|
|
Description Added ISO 8601 time format output.
Fixes issue 431.
Patch Set 1 #Patch Set 2 : code review 179079: Added ISO 8601 time format output. #Patch Set 3 : code review 179079: Added ISO 8601 time format output. #
Total comments: 3
Patch Set 4 : code review 179079: Added ISO 8601 time format output. #
Total comments: 5
Patch Set 5 : code review 179079: Added ISO 8601 time format output. #
Total comments: 2
Patch Set 6 : code review 179079: Added ISO 8601 time format output. #
Total comments: 5
Patch Set 7 : code review 179079: Added ISO 8601 time format output. #Patch Set 8 : code review 179079: Added ISO 8601 time format output. #MessagesTotal messages: 21
Hello golang-dev@googlegroups.com (cc: benolive, golang-dev@googlegroups.com), I'd like you to review the following change.
Sign in to reply to this message.
http://codereview.appspot.com/179079/diff/1005/1006 File src/pkg/time/time.go (right): http://codereview.appspot.com/179079/diff/1005/1006#newcode323 src/pkg/time/time.go:323: decimal(buf[bp:bp+2], t.Month); delete all the trailing semicolons http://codereview.appspot.com/179079/diff/1005/1006#newcode386 src/pkg/time/time.go:386: // date(1) - Sun Nov 6 08:49:37 UTC 1994 s/ -/:/ for consistency while you're here
Sign in to reply to this message.
> delete all the trailing semicolons cd $GOROOT/src hg sync all.bash hg gofmt
Sign in to reply to this message.
>> delete all the trailing semicolons > > cd $GOROOT/src > hg sync > all.bash > hg gofmt > That explains it. I think I ran hg gofmt after the initial upload. I have re uploaded with those changes now.
Sign in to reply to this message.
Removed the extraneous semicolons and changed that - to a comma http://codereview.appspot.com/179079/diff/1005/1006 File src/pkg/time/time.go (right): http://codereview.appspot.com/179079/diff/1005/1006#newcode323 src/pkg/time/time.go:323: decimal(buf[bp:bp+2], t.Month); On 2009/12/17 00:36:49, r wrote: > delete all the trailing semicolons Hmm, I went to fix this in my copy and the semicolons were gone. Is this something that `hg gofmt` would auto fix? I may have run that in the wrong order so it didn't get sent to the review.
Sign in to reply to this message.
looks good to me. russ? -rob
Sign in to reply to this message.
http://codereview.appspot.com/179079/diff/9/1012 File src/pkg/time/time.go (right): http://codereview.appspot.com/179079/diff/9/1012#newcode363 src/pkg/time/time.go:363: func (t *Time) ISO8601() (formatted string) { s/(formatted string)/string/ - matches the others and the name doesn't really matter here. http://codereview.appspot.com/179079/diff/9/1012#newcode365 src/pkg/time/time.go:365: formatted = format(t, "%Y-%m-%dT%H:%M:%SZ") return format(...) http://codereview.appspot.com/179079/diff/9/1012#newcode370 src/pkg/time/time.go:370: if t.ZoneOffset < 0 { in strftime this would be %z. let's move it into format as case 'z' above. it's sure to come up again. http://codereview.appspot.com/179079/diff/9/1012#newcode372 src/pkg/time/time.go:372: decimal(buf[bp:bp+2], int(-1*t.ZoneOffset/3600)) s/-1*/-/ http://codereview.appspot.com/179079/diff/9/1012#newcode373 src/pkg/time/time.go:373: decimal(buf[bp+2:bp+4], int(-1*t.ZoneOffset)%3600) s/-1*/-/
Sign in to reply to this message.
Ok, I moved the timezone stuff to %z in format. I also noticed my code wouldn't correctly handle timezones like -0930 so I went ahead and fixed that too.
Sign in to reply to this message.
http://codereview.appspot.com/179079/diff/13/14 File src/pkg/time/time.go (right): http://codereview.appspot.com/179079/diff/13/14#newcode339 src/pkg/time/time.go:339: decimal(buf[bp:bp+2], int(-t.ZoneOffset/3600)) shouldn't need the int conversion here and on the next line. you don't have them in the else case.
Sign in to reply to this message.
I don't know why I did that. The cast didn't seem necessary so I removed it. http://codereview.appspot.com/179079/diff/13/14 File src/pkg/time/time.go (right): http://codereview.appspot.com/179079/diff/13/14#newcode339 src/pkg/time/time.go:339: decimal(buf[bp:bp+2], int(-t.ZoneOffset/3600)) On 2009/12/17 01:41:13, rsc wrote: > shouldn't need the int conversion here and on the next line. > you don't have them in the else case. > Done.
Sign in to reply to this message.
Thanks. The code looks good; a couple nits on the tests. http://codereview.appspot.com/179079/diff/19/21 File src/pkg/time/time_test.go (right): http://codereview.appspot.com/179079/diff/19/21#newcode104 src/pkg/time/time_test.go:104: func TestISO8601Conversion(t *testing.T) { If there are two tests, there will be more. Make a table (see utctests and localtests for examples, above) and iterate over it. Then you can easily add a third test that has a time zone of +0420.
Sign in to reply to this message.
http://codereview.appspot.com/179079/diff/19/21 File src/pkg/time/time_test.go (right): http://codereview.appspot.com/179079/diff/19/21#newcode104 src/pkg/time/time_test.go:104: func TestISO8601Conversion(t *testing.T) { On 2009/12/17 02:00:08, rsc wrote: > If there are two tests, there will be more. Make a table > (see utctests and localtests for examples, above) and > iterate over it. > > Then you can easily add a third test that has a time zone > of +0420. > Ok, will do. Should I put the table at the top of the file or right above the Test?
Sign in to reply to this message.
> Ok, will do. Should I put the table at the top of the file or right > above the Test? above the test is fine
Sign in to reply to this message.
http://codereview.appspot.com/179079/diff/19/21 File src/pkg/time/time_test.go (right): http://codereview.appspot.com/179079/diff/19/21#newcode104 src/pkg/time/time_test.go:104: func TestISO8601Conversion(t *testing.T) { On 2009/12/17 03:40:48, benolive wrote: > On 2009/12/17 02:00:08, rsc wrote: > > If there are two tests, there will be more. Make a table > > (see utctests and localtests for examples, above) and > > iterate over it. > > > > Then you can easily add a third test that has a time zone > > of +0420. > > > > Ok, will do. Should I put the table at the top of the file or right above the > Test? Done.
Sign in to reply to this message.
almost there... http://codereview.appspot.com/179079/diff/19/21 File src/pkg/time/time_test.go (right): http://codereview.appspot.com/179079/diff/19/21#newcode104 src/pkg/time/time_test.go:104: func TestISO8601Conversion(t *testing.T) { On 2009/12/17 02:00:08, rsc wrote: > If there are two tests, there will be more. Make a table > (see utctests and localtests for examples, above) and > iterate over it. > > Then you can easily add a third test that has a time zone > of +0420. Please add a test with a timezone of +0420.
Sign in to reply to this message.
http://codereview.appspot.com/179079/diff/19/21 File src/pkg/time/time_test.go (right): http://codereview.appspot.com/179079/diff/19/21#newcode104 src/pkg/time/time_test.go:104: func TestISO8601Conversion(t *testing.T) { On 2009/12/17 04:27:12, rsc wrote: > On 2009/12/17 02:00:08, rsc wrote: > > If there are two tests, there will be more. Make a table > > (see utctests and localtests for examples, above) and > > iterate over it. > > > > Then you can easily add a third test that has a time zone > > of +0420. > > Please add a test with a timezone of +0420. > > Done.
Sign in to reply to this message.
Code looks great. Thanks for the quick turnarounds. Final hurdle: you need to complete one of the license agreements so we can use your code. See http://golang.org/doc/contribute.html#copyright Thanks. Russ
Sign in to reply to this message.
I did that earlier today. Is there a delay? --Ben On Thu, Dec 17, 2009 at 12:35 AM, <rsc@golang.org> wrote: > Code looks great. Thanks for the quick turnarounds. > > Final hurdle: you need to complete one of the license > agreements so we can use your code. See > http://golang.org/doc/contribute.html#copyright > > Thanks. > Russ > > > http://codereview.appspot.com/179079 >
Sign in to reply to this message.
> I did that earlier today. Is there a delay? No, sorry. I got faked out by the different codereview.appspot.com username and searched for benolive instead of sionide21. I see it now. Thanks. Russ
Sign in to reply to this message.
LGTM Will submit after 180081, currently out for review.
Sign in to reply to this message.
*** Submitted as http://code.google.com/p/go/source/detail?r=feb2d8a970ac *** time: add ISO 8601 time format Fixes issue 431. R=r, rsc CC=golang-dev http://codereview.appspot.com/179079 Committer: Russ Cox <rsc@golang.org>
Sign in to reply to this message.
|