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: Time.Format() should fail when the layout string is unexpected #34997

Closed
jqll opened this issue Oct 18, 2019 · 10 comments
Closed

time: Time.Format() should fail when the layout string is unexpected #34997

jqll opened this issue Oct 18, 2019 · 10 comments
Labels
FrozenDueToAge WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided.

Comments

@jqll
Copy link

jqll commented Oct 18, 2019

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

$ go version
go1.13

Does this issue reproduce with the latest release?

Yes

What operating system and processor architecture are you using (go env)?

go env Output
$ go env
GOHOSTOS="linux"

What did you do?

On 2019-10-18, run time.Now().Format("2006-01-01") (instead of the correct magic string "2006-01-02")

What did you expect to see?

  1. Go magically understands the format and returns the correct time with this format.
    or
  2. It complains the given format string is unexpected, either during compiling or at run time.

What did you see instead?

It returns a wrong time "2019-10-10".

@xhit
Copy link
Contributor

xhit commented Oct 18, 2019

Is not "magical".

When you write time.Now().Format("2006-01-01") you are printing the month twice, so the format and output are correct.

@bradfitz
Copy link
Contributor

Sorry, but this is working as designed & documented.

We're not going to add magic (it's not clear what magic would work), and we can't make it return an error on duplicate layout rules, as that would likely break other people's (correct, if odd) programs.

Maybe there could be a vet or lint check for bizarre and likely incorrect format strings, though.

I wouldn't be surprised if @dominikh has already written such a thing.

@dominikh
Copy link
Member

Actually I haven't. I would have the same issue on my end, trying to discern weird formats from incorrect formats.

Maybe one day editors will become powerful enough to show example formatted times in tooltips.

@jqll
Copy link
Author

jqll commented Oct 20, 2019

Thanks all! In the meantime, it's probably worth to explicitly warn in the doc https://golang.org/pkg/time/#Time.Format that the reference time must be "Mon Jan 2 15:04:05 -0700 MST 2006". Using a different time can lead to undefined behavior, not necessarily an error. Further explaining "01" = month, "02" = date will make things much more clear.

@bradfitz
Copy link
Contributor

It already says that at the URL you just linked to.

@jqll
Copy link
Author

jqll commented Oct 20, 2019

I wish the consequence of using an arbitrary date is explicitly documented. When I read the doc, I thought using an arbitrary date will either work or give an error. So I tried and thought it worked, as there was no error. The fact that "Mon Jan 2 15:04:05 -0700 MST 2006" itself looks arbitrary and is not explained also encourages the attempt.

I'm probably biased by my experience (which is largely due to my fault), but unintentionally returning wrong timestamp can be dangerous. I wish people are warned about it even if they just skim through the doc.

@ianlancetaylor
Copy link
Contributor

Do you have a specific suggestion for how the docs should be improved? It seems hard to fix the docs to work for people who just skim them. Thanks.

@jqll
Copy link
Author

jqll commented Oct 21, 2019

It would be very helpful if the reference time is explained and the fact that "01" = month and "02"= date is explicitly mentioned.

People can make different guesses of the underlying mechanism by reading "Format returns a textual representation of the time value formatted according to layout, which defines the format by showing how the reference time, defined to be Mon Jan 2 15:04:05 -0700 MST 2006 would be displayed if it were the value"

A very reasonable understanding is that the time should be "Mon Jan 2 15:04:05 -0700 MST 2006", so "2006-01-02 01:04:05 AMT" when expressing in AMT. But we get "2019-10-20 10:04:05 AMT" with the following code:

t, err := time.Parse("2006-01-02 15:04:05 MST", "2019-10-20 20:04:05 AMT")
if err != nil { return err }
fmt.Printf("%s", t.Format("2006-01-02 01:04:05 AMT"))

It's obvious once we know "01" = month and "02" = date. But otherwise it's surprising. I'm still worried that I can make this mistake without being warned. But making the doc very clear can help.

@dmitshur dmitshur changed the title Time.Format() should fail when the layout string is unexpected time: Time.Format() should fail when the layout string is unexpected Oct 21, 2019
@dmitshur dmitshur added the WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided. label Oct 21, 2019
@tv42
Copy link

tv42 commented Oct 28, 2019

@dominikh I'd think a rough heuristic for time formats to be that they shouldn't contain repeats of the same segment (01 etc). That'd be technically more lint than vet, unfortunately, because of joke code like

rebeccaBlack := time.Now().Format("It's Monday, Monday\nGotta get down on Monday")

But I really can't think of a non-joke example. Might be worth running through the corpus and just enabling it as a vet if nothing matches -- jokes can always add overrides or ignore vet results.

@gopherbot
Copy link

Timed out in state WaitingForInfo. Closing.

(I am just a bot, though. Please speak up if this is a mistake or you have the requested information.)

@golang golang locked and limited conversation to collaborators Nov 20, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided.
Projects
None yet
Development

No branches or pull requests

8 participants