Regression in parsing Duration flag

linki commented 1 year ago

https://github.com/alecthomas/kingpin/pull/329 introduced a difference in parsing a DurationVar compared to the old version.

Unfortunately, this breaks the old behaviour when the duration was a negative value. The most common use case is probably to have a Default("-1s") to denote that the value isn't set at all. Similarly a --some-duration="" would previously fail but is now treated as 0s. Here are the differences that I can observe:

// parse negative duration
time.ParseDuration("-1s") => `-1s` // no error
str2duration.Str2Duration("-1s") => `(*errors.errorString)(0x140001c2b70)(invalid input duration string)`

// parse empty string
time.ParseDuration("") => `(*errors.errorString)(0x140001c2b80)(time: invalid duration "")`
str2duration.Str2Duration("") => 0s // no error

In practice this leads to the following (some-duration is a DurationVar).

// old version (v2.2.6)
$ go run main.go --some-duration="-1s"
... // runs fine

// new version (v2.3.1 (2e61e1e))
$ go run main.go --some-duration="-1s"
main: error: invalid input duration string, try --help
exit status 1

// old version (v2.2.6)
$ go run main.go --some-duration=""
main: error: time: invalid duration "", try --help
exit status 1

// new version (v2.3.1 (2e61e1e))
$ go run main.go --some-duration=""
... // runs fine

/cc @adowair