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

time: cannot roundtrip Parse the Time.String output #20876

Closed
dsnet opened this issue Jul 1, 2017 · 4 comments
Closed

time: cannot roundtrip Parse the Time.String output #20876

dsnet opened this issue Jul 1, 2017 · 4 comments
Labels
Documentation FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Milestone

Comments

@dsnet
Copy link
Member

dsnet commented Jul 1, 2017

Regression from Go1.8.

On tip, the documentation of Time.String says:

String returns the time formatted using the format string: "2006-01-02 15:04:05.999999999 -0700 MST"

This is misleading since it suggests that the following should work:

time.Parse("2006-01-02 15:04:05.999999999 -0700 MST", time.Now().String())

However, this does not work on Go1.9 since:

parsing time "2017-06-30 17:31:36.969388848 -0700 PDT m=+0.004118679": extra text:  m=+0.004118679

We should done one of the following:

  • Say that round-trip parsing does not work and fix the documentation on Time.String to not suggest that the format string is "2006-01-02 15:04:05.999999999 -0700 MST". We can suggest that String is intended only for human consumption and is not meant to be machine readable.
  • Or fix the API for Parse, such that the monotonic timestamps can be ignored.

\cc @bradfitz @rsc

@dsnet dsnet added the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label Jul 1, 2017
@dsnet dsnet added this to the Go1.9 milestone Jul 1, 2017
@dsnet
Copy link
Member Author

dsnet commented Jul 1, 2017

I argue that we should have Parse ignore the monotonic measurements since there is already existing code that assumes that time.Parse("2006-01-02 15:04:05.999999999 -0700 MST", myTime.String()) should work.

@rsc
Copy link
Contributor

rsc commented Jul 6, 2017

The initial report is misleading. The full docs for time.Time.String say:

func (t Time) String() string
    String returns the time formatted using the format string

    "2006-01-02 15:04:05.999999999 -0700 MST"

    If the time has a monotonic clock reading, the returned string includes a
    final field "m=±<value>", where value is the monotonic clock reading
    formatted as a decimal number of seconds.

Are you aware of anyone round-tripping the output of String in production code? I am not. It's a mistake anyway since the String output gives the time zone twice, the second one will override the first one, and it's the less specific of the two.

If there are widespread production uses of String output being fed into time.Parse, let me know.

Otherwise, if someone wants to add to the end of the doc comment that

The returned string is meant for debugging; for a stable serialized representation,
use t.MarshalText, t.MarshalBinary, or t.Format with an explicit format string.

that's fine.

@bradfitz bradfitz added Documentation NeedsFix The path to resolution is known, but the work has not been done. and removed NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. labels Jul 6, 2017
@gopherbot
Copy link

CL https://golang.org/cl/47552 mentions this issue.

@dsnet
Copy link
Member Author

dsnet commented Jul 6, 2017

SGTM. It was an assumption made by several tests. Fixing them will be easy.

k8s-github-robot pushed a commit to kubernetes/kubernetes that referenced this issue Apr 12, 2018
…stamp

Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>.

Fix parsing timestamp in test

Go 1.9 changed the default string format, see golang/go#20876 

As discussed there, default format is intended to be readable for humans and can change without warning, so we should probably set the format explicitly when writing status configmap.

```release-note
NONE
```
@golang golang locked and limited conversation to collaborators Sep 6, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Documentation FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Development

No branches or pull requests

4 participants