Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

proposal: time: Duration String() should omit zero units #39064

Closed
XSAM opened this issue May 14, 2020 · 7 comments
Closed

proposal: time: Duration String() should omit zero units #39064

XSAM opened this issue May 14, 2020 · 7 comments
Labels
Projects
Milestone

Comments

@XSAM
Copy link
Contributor

@XSAM XSAM commented May 14, 2020

What version of Go are you using (go version)?

1.14.2

Proposal

I propose that time.Duration.Strings() should omit zero units.

For example

fmt.Println((5 * time.Hour).String())

With this proposal, this example result will change from 5h0m0s to 5h.

  1. It's more concise. Zero units like 0m0s is meaningless.
  2. Currently, we already omit the ns, µs and ms units if its value is zero, so we should also emit the s and h unit to keep the String() behavior more consistent and predictable.
@gopherbot gopherbot added this to the Proposal milestone May 14, 2020
@gopherbot gopherbot added the Proposal label May 14, 2020
@ianlancetaylor ianlancetaylor changed the title Proposal: time.Duration String() should omit zero units proposal: time: Duration String() should omit zero units May 14, 2020
@ianlancetaylor ianlancetaylor added this to Incoming in Proposals May 14, 2020
@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented May 14, 2020

Unfortunately, at this point this seems likely to break some existing programs. Is the benefit worth the cost?

@XSAM
Copy link
Contributor Author

@XSAM XSAM commented May 14, 2020

Well, it will not break programs which try to parse to the duration since time.ParseDuration already handle these values.

duration, _ := time.ParseDuration("5h")
if duration == 5*time.Hour {
	fmt.Println("equal")
}

But, it will break programs that try to stringify the Duration and check the result.


This proposal is a sort of enhancing the Duration.String, which makes it more elegant. It may break some existing programs, and I not sure what the actual cost is.

If the Go team thinks that the cost is not acceptable, feel free to close it.

@cespare
Copy link
Contributor

@cespare cespare commented May 14, 2020

At work we have a time.Duration printing function that trims the useless 0s and 0m suffixes. This is almost always a nicer way to print durations that were provided by humans (for instance, as a flag.Duration).

If we can't change the behavior of String, perhaps we could add a new method. (Display, ShortString, Str...? Nothing great is coming to mind.)

@rsc
Copy link
Contributor

@rsc rsc commented May 20, 2020

FWIW, we used to print just "0" for zero and changed it to "0s" a while back. #14058. (Not the same as this issue but related.)

It looks like right now the implementation has the invariant that it always prints a string ending in "s" - that is, there is always some number of seconds reported. Maybe a smaller unit, but always some kind of seconds. If larger units can be factored out, they are printed ahead of the seconds.

The docs are also pretty clear:

func (d Duration) String() string
    String returns a string representing the duration in the form "72h3m0.5s".
    Leading zero units are omitted. As a special case, durations less than one
    second format use a smaller unit (milli-, micro-, or nanoseconds) to ensure
    that the leading digit is non-zero. The zero duration formats as 0s.

It seems almost certain that changing this would break tests, and probably also libraries, and silently.

For better or worse, this is the documented format. It's too late to change.

@rsc rsc moved this from Incoming to Active in Proposals May 20, 2020
@rsc
Copy link
Contributor

@rsc rsc commented May 27, 2020

Based on the discussion above, this seems like a likely decline.

@rsc rsc moved this from Active to Likely Decline in Proposals May 27, 2020
@ainar-g
Copy link
Contributor

@ainar-g ainar-g commented May 27, 2020

@cespare I agree that a function like Display is needed, but I also think that that function should probably be in an i18n package. How about somewhere in golang.org/x/text?

@rsc
Copy link
Contributor

@rsc rsc commented Jun 3, 2020

No change in consensus, so declined.

@ainar-g, exactly what you want to display changes from team to team and maybe project to project. The best place for this logic is in your own code, or in a third-party library if you want to share it. (Personally I always use fmt.Sprintf("%.3fs", d.Seconds()), but I'm not adding that to the standard library either.)

@rsc rsc closed this Jun 3, 2020
@rsc rsc moved this from Likely Decline to Declined in Proposals Jun 3, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Proposals
Declined
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
6 participants
You can’t perform that action at this time.