code review 179079: Added ISO 8601 time format output. (issue179079)

[sion...@gmail.com]

Reviewers: golang-dev_googlegroups.com,

Message:
Hello golan...@googlegroups.com (cc: benolive,
golan...@googlegroups.com),

I'd like you to review the following change.


Description:
Added ISO 8601 time format output.
Fixes issue 431.

Please review this at http://codereview.appspot.com/179079

Affected files:
M src/pkg/time/time.go
M src/pkg/time/time_test.go


Index: src/pkg/time/time.go
===================================================================
--- a/src/pkg/time/time.go
+++ b/src/pkg/time/time.go
@@ -319,6 +319,9 @@
case 'M': // %M minute 00-59
decimal(buf[bp:bp+2], t.Minute)
bp += 2
+ case 'm': // %m month 01-12
+ decimal(buf[bp:bp+2], t.Month);
+ bp += 2;
case 'S': // %S second 00-59
decimal(buf[bp:bp+2], t.Second)
bp += 2
@@ -355,6 +358,30 @@
// RFC 1123: Sun, 06 Nov 1994 08:49:37 UTC
func (t *Time) RFC1123() string { return
format(t, "%a, %d %b %Y %H:%M:%S %Z") }

+// ISO8601 formats the parsed time value in the style of
+// ISO 8601: 1994-11-06T08:49:37Z
+func (t *Time) ISO8601() (formatted string) {
+ if t.ZoneOffset == 0 {
+ formatted = format(t, "%Y-%m-%dT%H:%M:%SZ")
+ } else {
+ buf := make([]byte, 25);
+ bp := addString(buf, 0, "%Y-%m-%dT%H:%M:%S");
+ // Create the tz in the form -0500
+ if t.ZoneOffset < 0 {
+ bp = addString(buf, bp, "-");
+ decimal(buf[bp:bp+2], int(-1*t.ZoneOffset/3600));
+ decimal(buf[bp+2:bp+4], int(-1*t.ZoneOffset)%3600);
+ } else {
+ bp = addString(buf, bp, "+");
+ decimal(buf[bp:bp+2], t.ZoneOffset/3600);
+ decimal(buf[bp+2:bp+4], t.ZoneOffset%3600);
+ }
+ bp += 4;
+ formatted = format(t, string(buf[0:bp]));
+ }
+ return;
+}
+
// String formats the parsed time value in the style of
// date(1) - Sun Nov 6 08:49:37 UTC 1994
func (t *Time) String() string { return
format(t, "%a %b %e %H:%M:%S %Z %Y") }
Index: src/pkg/time/time_test.go
===================================================================
--- a/src/pkg/time/time_test.go
+++ b/src/pkg/time/time_test.go
@@ -101,6 +101,23 @@
}
}

+func TestISO8601Conversion(t *testing.T) {
+ timeA := Time{2008, 9, 17, 20, 4, 26, Wednesday, 0, "UTC"};
+ timeB := Time{1994, 9, 17, 20, 4, 26, Wednesday, -18000, "EST"};
+
+ if timeA.ISO8601() != "2008-09-17T20:04:26Z" {
+ t.Error("ISO8601():");
+ t.Errorf(" want=%+v", "2008-09-17T20:04:26Z");
+ t.Errorf(" have=%+v", timeA.ISO8601());
+ }
+
+ if timeB.ISO8601() != "1994-09-17T20:04:26-0500" {
+ t.Error("ISO8601():");
+ t.Errorf(" want=%+v", "1994-09-17T20:04:26-0500");
+ t.Errorf(" have=%+v", timeB.ISO8601());
+ }
+}
+
func BenchmarkSeconds(b *testing.B) {
for i := 0; i < b.N; i++ {
Seconds()

[r...@golang.org]

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

http://codereview.appspot.com/179079

[Russ Cox]

> delete all the trailing semicolons

cd $GOROOT/src
hg sync
all.bash
hg gofmt

[Ben Olive]

That explains it. I think I ran hg gofmt after the initial upload. I
have re uploaded with those changes now.

[Rob 'Commander' Pike]

looks good to me. russ?

-rob

[r...@golang.org]

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*/-/

http://codereview.appspot.com/179079

[sion...@gmail.com]

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.

http://codereview.appspot.com/179079

[r...@golang.org]

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.

http://codereview.appspot.com/179079

[sion...@gmail.com]

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.

http://codereview.appspot.com/179079

[r...@golang.org]

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.

http://codereview.appspot.com/179079

[sion...@gmail.com]

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?

http://codereview.appspot.com/179079

[Russ Cox]

> Ok, will do. Should I put the table at the top of the file or right
> above the Test?

above the test is fine

[sion...@gmail.com]

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.

http://codereview.appspot.com/179079

[r...@golang.org]

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.

http://codereview.appspot.com/179079

[sion...@gmail.com]

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.

http://codereview.appspot.com/179079

[r...@golang.org]

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

[Ben Olive]

I did that earlier today. Is there a delay?

--Ben

[Russ Cox]

> 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

[r...@golang.org]

LGTM

Will submit after 180081, currently out for review.

http://codereview.appspot.com/179079

[r...@golang.org]

*** 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 <r...@golang.org>

http://codereview.appspot.com/179079