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

Comments

@dsnet
Copy link
Member

@dsnet 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 label Jul 1, 2017
@dsnet dsnet added this to the Go1.9 milestone Jul 1, 2017
@dsnet
Copy link
Member Author

@dsnet 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 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.

@gopherbot
Copy link

@gopherbot gopherbot commented Jul 6, 2017

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

@dsnet
Copy link
Member Author

@dsnet dsnet commented Jul 6, 2017

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

@gopherbot gopherbot closed this in 4e2eff4 Jul 6, 2017
k8s-github-robot pushed a commit to kubernetes/kubernetes that referenced this issue Apr 12, 2018
Kubernetes Submit Queue
…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.
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

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