code review 4830065: time: parse and format fractional seconds

time: parse and format fractional seconds

[r, 2011-08-07 02:05:33]

Hello golang-dev@googlegroups.com,

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

[rog, 2011-08-07 02:52:31]

LGTM

http://codereview.appspot.com/4830065/diff/1/src/pkg/time/format.go
File src/pkg/time/format.go (right):

http://codereview.appspot.com/4830065/diff/1/src/pkg/time/format.go#newcode668
src/pkg/time/format.go:668: t.Nanosecond, err = strconv.Atoi(value[1:len(std)])
Atoui ?

[r2, 2011-08-07 02:54:58]

On 07/08/2011, at 12:52 PM, rogpeppe@gmail.com wrote:

> LGTM
> 
> 
> http://codereview.appspot.com/4830065/diff/1/src/pkg/time/format.go
> File src/pkg/time/format.go (right):
> 
> http://codereview.appspot.com/4830065/diff/1/src/pkg/time/format.go#newcode668
> src/pkg/time/format.go:668: t.Nanosecond, err =
> strconv.Atoi(value[1:len(std)])
> Atoui ?

done

[dsymonds, 2011-08-07 05:46:06]

http://codereview.appspot.com/4830065/diff/5001/src/pkg/time/format.go
File src/pkg/time/format.go (right):

http://codereview.appspot.com/4830065/diff/5001/src/pkg/time/format.go#newcode30
src/pkg/time/format.go:30: // fractional second.
only when following seconds? isn't that what we settled on?

Otherwise, how do you format, say, foo.MM.DD? You'd previously do "foo.01.02",
but now that wouldn't be possible.

[r2, 2011-08-07 05:56:11]

On 07/08/2011, at 3:46 PM, dsymonds@golang.org wrote:

> 
> http://codereview.appspot.com/4830065/diff/5001/src/pkg/time/format.go
> File src/pkg/time/format.go (right):
> 
>
http://codereview.appspot.com/4830065/diff/5001/src/pkg/time/format.go#newcode30
> src/pkg/time/format.go:30: // fractional second.
> only when following seconds? isn't that what we settled on?
> 
> Otherwise, how do you format, say, foo.MM.DD? You'd previously do
> "foo.01.02", but now that wouldn't be possible.

"foo.01.02" works fine. the fractional decimal case is tried last.

-rob

[dsymonds, 2011-08-07 06:03:45]

On Sun, Aug 7, 2011 at 3:55 PM, Rob 'Commander' Pike <r@google.com> wrote:

> "foo.01.02" works fine. the fractional decimal case is tried last.

Can you add a test case for that?

[bradfitz, 2011-08-07 06:16:38]

The bug that I and many others have hit is the RFC3339 format.

RFC3339 can sometimes contain fractional seconds, and sometimes not.

Can the time.RFC3339 format constant support both variants?  (if so, a new test
line for 3339 with fractional second would be nice)

[fvb, 2011-08-07 06:53:52]

http://codereview.appspot.com/4830065/diff/4003/src/pkg/time/format.go
File src/pkg/time/format.go (right):

http://codereview.appspot.com/4830065/diff/4003/src/pkg/time/format.go#newcode29
src/pkg/time/format.go:29: // A decimal point followed by one or more zeros
represents a
What about cultures that use decimal *commas*?

[r2, 2011-08-07 07:44:19]

On 07/08/2011, at 4:53 PM, fvbommel@gmail.com wrote:

> 
> http://codereview.appspot.com/4830065/diff/4003/src/pkg/time/format.go
> File src/pkg/time/format.go (right):
> 
>
http://codereview.appspot.com/4830065/diff/4003/src/pkg/time/format.go#newcode29
> src/pkg/time/format.go:29: // A decimal point followed by one or more
> zeros represents a
> What about cultures that use decimal *commas*?

That is a much smaller problem than Feb not being the international standard for
the second month. Not to make light of the issue, but some big localization
changes are needed in the library - and will happen - and picking off one
special case isn't the place to start.  The fmt package doesn't get this right,
either.

-rob

[r2, 2011-08-07 07:45:35]

On 07/08/2011, at 4:16 PM, bradfitz@golang.org wrote:

> The bug that I and many others have hit is the RFC3339 format.
> 
> RFC3339 can sometimes contain fractional seconds, and sometimes not.
> 
> Can the time.RFC3339 format constant support both variants?  (if so, a
> new test line for 3339 with fractional second would be nice)

time.RFC3339 cannot at the moment. Another format could.

The other approach is to parse (only) fractional seconds even if the format
doesn't have them. I've been debating doing that, but I'd rather do that in a
separate CL since it's a symmetry break.

-rob

[r2, 2011-08-07 08:04:31]

On 07/08/2011, at 4:03 PM, David Symonds wrote:

> On Sun, Aug 7, 2011 at 3:55 PM, Rob 'Commander' Pike <r@google.com> wrote:
> 
>> "foo.01.02" works fine. the fractional decimal case is tried last.
> 
> Can you add a test case for that?

Done, and your simple case worked but others did not. Fixed without needing the
"after seconds" requirement.

-rob

[r, 2011-08-07 08:04:46]

Hello golang-dev@googlegroups.com, rogpeppe@gmail.com, r@google.com,
dsymonds@golang.org, bradfitz@golang.org, fvbommel@gmail.com (cc:
golang-dev@googlegroups.com),

Please take another look.

[r, 2011-08-07 08:06:57]

Hello golang-dev@googlegroups.com, rogpeppe@gmail.com, r@google.com,
dsymonds@golang.org, bradfitz@golang.org, fvbommel@gmail.com (cc:
golang-dev@googlegroups.com),

Please take another look.

[r, 2011-08-07 08:09:16]

Hello golang-dev@googlegroups.com, rogpeppe@gmail.com, r@google.com,
dsymonds@golang.org, bradfitz@golang.org, fvbommel@gmail.com (cc:
golang-dev@googlegroups.com),

Please take another look.

[dsymonds, 2011-08-07 13:34:10]

LGTM

[r, 2011-08-07 21:50:32]

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

time: parse and format fractional seconds

R=golang-dev, rogpeppe, r, dsymonds, bradfitz, fvbommel
CC=golang-dev
http://codereview.appspot.com/4830065

[rsc, 2011-08-08 13:32:11]

I think this might be too strict.

I had hoped for the logical item to stay 'seconds',
so that "05", "05.0", "05.0000" all denote the
'seconds chunk', with the digit count being a
precision used by Format but ignored by Parse.

It seems likely that most places where we
might want to allow parsing of fractional seconds,
we would also want to allow parsing without them
or with a different number of significant digits.

Russ

[r2, 2011-08-08 13:42:26]

On 08/08/2011, at 11:32 PM, Russ Cox wrote:

> I think this might be too strict.
> 
> I had hoped for the logical item to stay 'seconds',
> so that "05", "05.0", "05.0000" all denote the
> 'seconds chunk', with the digit count being a
> precision used by Format but ignored by Parse.
> 
> It seems likely that most places where we
> might want to allow parsing of fractional seconds,
> we would also want to allow parsing without them
> or with a different number of significant digits.

Perhaps, but you also need to respect the format when you parse, because you
must be able to use the same format string for parsing as for printing.  What
you're asking is therefore a new feature, not a change to the existing behavior,
and it's a change I've been pondering since before this work.

In other words, the only thing that your (and Brad's) request is asking for is
accepting optional decimal followed by digits when parsing the seconds field.
Put that way, I think you can see that's a little dangerous or at least tricky 
(15.23.14.02.01.59.2006), which is why I'm still mulling it over.  But such
cases may be unlikely or unimportant enough to ignore.

There are other approaches possible. There could be a different entry point for
this case, or there could be a special "optional fraction" format indicator.

In summary, I don't think it's as clear cut as you make out, and what's there
now needs to be there anyway.

-rob