Issue 5489111: code review 5489111: time: add ParseDuration. (Closed)
|
Created:
13 years, 7 months ago by dsymonds Modified:
13 years, 7 months ago Reviewers:
CC:
rsc, r, r2, golang-dev Visibility:
Public. |
Descriptiontime: add ParseDuration.
Patch Set 1 #Patch Set 2 : diff -r 747c04659b0f https://go.googlecode.com/hg/ #
Total comments: 6
Patch Set 3 : diff -r 747c04659b0f https://go.googlecode.com/hg/ #
Total comments: 14
Patch Set 4 : diff -r 747c04659b0f https://go.googlecode.com/hg/ #
Total comments: 6
Patch Set 5 : diff -r 747c04659b0f https://go.googlecode.com/hg/ #
Total comments: 4
Patch Set 6 : diff -r aa99dd6bc07a https://go.googlecode.com/hg/ #
Total comments: 2
Patch Set 7 : diff -r 6bb207d24ffa https://go.googlecode.com/hg/ #MessagesTotal messages: 14
Hello rsc (cc: golang-dev@googlegroups.com), I'd like you to review this change to https://go.googlecode.com/hg/
Sign in to reply to this message.
http://codereview.appspot.com/5489111/diff/1002/src/pkg/time/format.go File src/pkg/time/format.go (right): http://codereview.appspot.com/5489111/diff/1002/src/pkg/time/format.go#newcod... src/pkg/time/format.go:901: // doesn't parse a sign, and allows for an empty string. you should be able to combine the code between here and atoi. both can return the reside; atoi can check that it's empty, you can check that it's a known symbol http://codereview.appspot.com/5489111/diff/1002/src/pkg/time/format.go#newcod... src/pkg/time/format.go:921: "µs": Microsecond, for erroneous completeness, admit μ (3bc) as well. http://codereview.appspot.com/5489111/diff/1002/src/pkg/time/format.go#newcod... src/pkg/time/format.go:968: if s != "" && s[0] == ' ' { don't accept spaces. "3s" is fine, "3 s" is not.
Sign in to reply to this message.
http://codereview.appspot.com/5489111/diff/1002/src/pkg/time/format.go File src/pkg/time/format.go (right): http://codereview.appspot.com/5489111/diff/1002/src/pkg/time/format.go#newcod... src/pkg/time/format.go:901: // doesn't parse a sign, and allows for an empty string. On 2011/12/22 23:24:16, r wrote: > you should be able to combine the code between here and atoi. both can return > the reside; atoi can check that it's empty, you can check that it's a known > symbol Done. http://codereview.appspot.com/5489111/diff/1002/src/pkg/time/format.go#newcod... src/pkg/time/format.go:921: "µs": Microsecond, On 2011/12/22 23:24:16, r wrote: > for erroneous completeness, admit μ (3bc) as well. Done. http://codereview.appspot.com/5489111/diff/1002/src/pkg/time/format.go#newcod... src/pkg/time/format.go:968: if s != "" && s[0] == ' ' { On 2011/12/22 23:24:16, r wrote: > don't accept spaces. "3s" is fine, "3 s" is not. Done.
Sign in to reply to this message.
http://codereview.appspot.com/5489111/diff/4001/src/pkg/time/format.go File src/pkg/time/format.go (right): http://codereview.appspot.com/5489111/diff/4001/src/pkg/time/format.go#newcod... src/pkg/time/format.go:892: // doesn't parse a sign, and allows for an empty string. comment is now wrong http://codereview.appspot.com/5489111/diff/4001/src/pkg/time/format.go#newcod... src/pkg/time/format.go:912: "µs": Microsecond, // U+00B5 s/$/ = micro symbol http://codereview.appspot.com/5489111/diff/4001/src/pkg/time/format.go#newcod... src/pkg/time/format.go:913: "μs": Microsecond, // U+03BC s/$/ = greek mu/ http://codereview.appspot.com/5489111/diff/4001/src/pkg/time/format.go#newcod... src/pkg/time/format.go:921: // A duration string consists of a floating point number and a time unit. A duration string is a possibly signed decimal number, with optional fraction, and a unit suffix, such as "300ms" or "-1.5h". (don't say floating point - you don't accept 1e9) http://codereview.appspot.com/5489111/diff/4001/src/pkg/time/format.go#newcod... src/pkg/time/format.go:959: // Consume [a-z]+ odd comment. // Remainder of string must be a unit. http://codereview.appspot.com/5489111/diff/4001/src/pkg/time/format.go#newcod... src/pkg/time/format.go:968: return Duration(f * float64(unit)), nil you could move the unit conversion into the map. http://codereview.appspot.com/5489111/diff/4001/src/pkg/time/time_test.go File src/pkg/time/time_test.go (right): http://codereview.appspot.com/5489111/diff/4001/src/pkg/time/time_test.go#new... src/pkg/time/time_test.go:845: } need some error cases "" "3" "-" "s"
Sign in to reply to this message.
http://codereview.appspot.com/5489111/diff/4001/src/pkg/time/format.go File src/pkg/time/format.go (right): http://codereview.appspot.com/5489111/diff/4001/src/pkg/time/format.go#newcod... src/pkg/time/format.go:892: // doesn't parse a sign, and allows for an empty string. On 2011/12/22 23:41:37, r wrote: > comment is now wrong Done. http://codereview.appspot.com/5489111/diff/4001/src/pkg/time/format.go#newcod... src/pkg/time/format.go:912: "µs": Microsecond, // U+00B5 On 2011/12/22 23:41:37, r wrote: > s/$/ = micro symbol Done. http://codereview.appspot.com/5489111/diff/4001/src/pkg/time/format.go#newcod... src/pkg/time/format.go:913: "μs": Microsecond, // U+03BC On 2011/12/22 23:41:37, r wrote: > s/$/ = greek mu/ Done. http://codereview.appspot.com/5489111/diff/4001/src/pkg/time/format.go#newcod... src/pkg/time/format.go:921: // A duration string consists of a floating point number and a time unit. On 2011/12/22 23:41:37, r wrote: > A duration string is a possibly signed decimal number, with optional fraction, > and a unit suffix, > such as "300ms" or "-1.5h". > > (don't say floating point - you don't accept 1e9) Done. http://codereview.appspot.com/5489111/diff/4001/src/pkg/time/format.go#newcod... src/pkg/time/format.go:959: // Consume [a-z]+ On 2011/12/22 23:41:37, r wrote: > odd comment. > // Remainder of string must be a unit. Done, but with different words. The next revision of this code will add support for multiple units (e.g. "5h30m"). http://codereview.appspot.com/5489111/diff/4001/src/pkg/time/format.go#newcod... src/pkg/time/format.go:968: return Duration(f * float64(unit)), nil On 2011/12/22 23:41:38, r wrote: > you could move the unit conversion into the map. Done. http://codereview.appspot.com/5489111/diff/4001/src/pkg/time/time_test.go File src/pkg/time/time_test.go (right): http://codereview.appspot.com/5489111/diff/4001/src/pkg/time/time_test.go#new... src/pkg/time/time_test.go:845: } On 2011/12/22 23:41:38, r wrote: > need some error cases > > "" > "3" > "-" > "s" Done.
Sign in to reply to this message.
LGTM leaving for rsc http://codereview.appspot.com/5489111/diff/5003/src/pkg/time/format.go File src/pkg/time/format.go (right): http://codereview.appspot.com/5489111/diff/5003/src/pkg/time/format.go#newcod... src/pkg/time/format.go:911: "μs": float64(Microsecond), // U+03BC = greek mu s/greek/Greek letter/ http://codereview.appspot.com/5489111/diff/5003/src/pkg/time/format.go#newcod... src/pkg/time/format.go:922: // TODO(dsymonds): Handle multiple units (e.g. "5m30s"). move the TODO into the function or just delete it. http://codereview.appspot.com/5489111/diff/5003/src/pkg/time/time_test.go File src/pkg/time/time_test.go (right): http://codereview.appspot.com/5489111/diff/5003/src/pkg/time/time_test.go#new... src/pkg/time/time_test.go:835: {".5s", true, 500 * Millisecond}, need a test with zeros in the fraction, like 1.0 1.00 1.005 and 1.0050 and 100.00100
Sign in to reply to this message.
http://codereview.appspot.com/5489111/diff/5003/src/pkg/time/format.go File src/pkg/time/format.go (right): http://codereview.appspot.com/5489111/diff/5003/src/pkg/time/format.go#newcod... src/pkg/time/format.go:911: "μs": float64(Microsecond), // U+03BC = greek mu On 2011/12/23 00:48:57, r wrote: > s/greek/Greek letter/ Done. http://codereview.appspot.com/5489111/diff/5003/src/pkg/time/format.go#newcod... src/pkg/time/format.go:922: // TODO(dsymonds): Handle multiple units (e.g. "5m30s"). On 2011/12/23 00:48:57, r wrote: > move the TODO into the function or just delete it. Done. http://codereview.appspot.com/5489111/diff/5003/src/pkg/time/time_test.go File src/pkg/time/time_test.go (right): http://codereview.appspot.com/5489111/diff/5003/src/pkg/time/time_test.go#new... src/pkg/time/time_test.go:835: {".5s", true, 500 * Millisecond}, On 2011/12/23 00:48:57, r wrote: > need a test with zeros in the fraction, like 1.0 1.00 1.005 and 1.0050 and > 100.00100 Done. Used slightly different numbers for the sake of floating point precision.
Sign in to reply to this message.
http://codereview.appspot.com/5489111/diff/1003/src/pkg/time/format.go File src/pkg/time/format.go (right): http://codereview.appspot.com/5489111/diff/1003/src/pkg/time/format.go#newcod... src/pkg/time/format.go:923: // TODO(dsymonds): Handle multiple units (e.g. "5m30s"). This should just be a loop around lines 938-967, right? Seems worth getting in in this CL. Then you can also test that random values turned from Duration -> String are ParseDuration'able back to Duration. http://codereview.appspot.com/5489111/diff/1003/src/pkg/time/time_test.go File src/pkg/time/time_test.go (right): http://codereview.appspot.com/5489111/diff/1003/src/pkg/time/time_test.go#new... src/pkg/time/time_test.go:819: var parseDurationTests = []struct { "0" is valid and should be tested. So are "+0" and "-0". "." is invalid, as are "-.", "+.", ".s", "+.s", and so on. They should be tested too.
Sign in to reply to this message.
On Dec 22, 2011, at 7:22 PM, rsc@golang.org wrote: > > http://codereview.appspot.com/5489111/diff/1003/src/pkg/time/format.go > File src/pkg/time/format.go (right): > > http://codereview.appspot.com/5489111/diff/1003/src/pkg/time/format.go#newcod... > src/pkg/time/format.go:923: // TODO(dsymonds): Handle multiple units > (e.g. "5m30s"). > This should just be a loop around lines 938-967, right? > Seems worth getting in in this CL. yeah, i had the same idea on the drive home. > Then you can also test that random values turned from > Duration -> String are ParseDuration'able back to Duration. and that. > http://codereview.appspot.com/5489111/diff/1003/src/pkg/time/time_test.go > File src/pkg/time/time_test.go (right): > > http://codereview.appspot.com/5489111/diff/1003/src/pkg/time/time_test.go#new... > src/pkg/time/time_test.go:819: var parseDurationTests = []struct { > "0" is valid and should be tested. > So are "+0" and "-0". > > "." is invalid, as are "-.", "+.", ".s", "+.s", and so on. > They should be tested too. > > http://codereview.appspot.com/5489111/
Sign in to reply to this message.
lgtm once those tests and the random round trips pass. leaving for rob or for january.
Sign in to reply to this message.
All done. I'll submit this shortly. It's not in the weekly, and nothing except the new flag type is depending on it, so we can come back and fix it up if I missed anything. http://codereview.appspot.com/5489111/diff/1003/src/pkg/time/format.go File src/pkg/time/format.go (right): http://codereview.appspot.com/5489111/diff/1003/src/pkg/time/format.go#newcod... src/pkg/time/format.go:923: // TODO(dsymonds): Handle multiple units (e.g. "5m30s"). On 2011/12/23 03:22:58, rsc wrote: > This should just be a loop around lines 938-967, right? > Seems worth getting in in this CL. > > Then you can also test that random values turned from > Duration -> String are ParseDuration'able back to Duration. Done. http://codereview.appspot.com/5489111/diff/1003/src/pkg/time/time_test.go File src/pkg/time/time_test.go (right): http://codereview.appspot.com/5489111/diff/1003/src/pkg/time/time_test.go#new... src/pkg/time/time_test.go:819: var parseDurationTests = []struct { On 2011/12/23 03:22:58, rsc wrote: > "0" is valid and should be tested. > So are "+0" and "-0". > > "." is invalid, as are "-.", "+.", ".s", "+.s", and so on. > They should be tested too. I've done a representative set of those.
Sign in to reply to this message.
LGTM http://codereview.appspot.com/5489111/diff/5006/src/pkg/time/format.go File src/pkg/time/format.go (right): http://codereview.appspot.com/5489111/diff/5006/src/pkg/time/format.go#newcod... src/pkg/time/format.go:920: // decimal number, with optional fraction and a unit suffix, // decimal numbers, each with ...
Sign in to reply to this message.
http://codereview.appspot.com/5489111/diff/5006/src/pkg/time/format.go File src/pkg/time/format.go (right): http://codereview.appspot.com/5489111/diff/5006/src/pkg/time/format.go#newcod... src/pkg/time/format.go:920: // decimal number, with optional fraction and a unit suffix, On 2011/12/23 05:12:59, r wrote: > // decimal numbers, each with ... Done.
Sign in to reply to this message.
*** Submitted as http://code.google.com/p/go/source/detail?r=84d26aa7161e *** time: add ParseDuration. R=rsc, r, r CC=golang-dev http://codereview.appspot.com/5489111
Sign in to reply to this message.
|
