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: Format() ignores _ on go tip #23259

Closed
fsouza opened this issue Dec 27, 2017 · 8 comments
Closed

time: Format() ignores _ on go tip #23259

fsouza opened this issue Dec 27, 2017 · 8 comments
Labels
FrozenDueToAge NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. release-blocker
Milestone

Comments

@fsouza
Copy link
Contributor

fsouza commented Dec 27, 2017

Please answer these questions before submitting your issue. Thanks!

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

% go version
go version devel +acce8268b6 Wed Dec 27 15:03:09 2017 +0000 darwin/amd64

Does this issue reproduce with the latest release?

Only on master.

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

% go env
GOARCH="amd64"
GOBIN=""
GOCACHE="/Users/fsouza/Library/Caches/go-build"
GOEXE=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GOOS="darwin"
GOPATH="/Users/fsouza"
GORACE=""
GOROOT="/Users/fsouza/.gimme/versions/go"
GOTMPDIR=""
GOTOOLDIR="/Users/fsouza/.gimme/versions/go/pkg/tool/darwin_amd64"
GCCGO="gccgo"
CC="clang"
CXX="clang++"
CGO_ENABLED="1"
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/t4/s5cn564x04j7s6nz7w_8cd3c0000gp/T/go-build094937801=/tmp/go-build -gno-record-gcc-switches -fno-common"

What did you do?

https://play.golang.org/p/1ccgLvVk8Jx

What did you expect to see?

20171227_122425

What did you see instead?

2017122712252425 (no _).

More info

It works fine on Go 1.9.2 and Go 1.10beta1:

% go version
go version devel +acce8268b6 Wed Dec 27 15:03:09 2017 +0000 darwin/amd64
% go run format.go
2017122712252425
% go version
go version go1.9.2 darwin/amd64
% go run format.go
20171227_122425
% go version
go version go1.10beta1 darwin/amd64
% go run format.go
20171227_122425
@nussjustin
Copy link
Contributor

Git bisect points to 8776be1

@ianlancetaylor
Copy link
Contributor

That change (https://golang.org/cl/78735) changes the string _1 to mean the month with space padding. Since your format string 20060102_150405 contains _1, the generated result has changed. Of course you meant it to see 15 rather than _1.

Since the use of _1 is a bit obscure, my inclination is to roll back the change rather than break (admittedly obscure) existing code. But we could also document this clearly in the release notes.

Any other opinions? CC @robpike @hallazzang

@ianlancetaylor ianlancetaylor added this to the Go1.10 milestone Dec 27, 2017
@ianlancetaylor ianlancetaylor added the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label Dec 27, 2017
@hallazzang
Copy link
Contributor

I know breaking someone's existing code is not good, I just wanted to make consistent API as I said in #22802. Since the result of this code(https://play.golang.org/p/3fAVlR36KIf) also seems not proper for you(on go 1.9.2), I can't find the best solution for this problem.

Here are what I can imagine as the second best solutions:

  • Introduce a special escape character like \ as in fmt.Printf
    • Yes, it sounds ugly
  • Use other characters for spacing character, like ^ that can be considered as rarely used in time layout, instead of current _.
  • Leave the time.Format as it was before that change, and add new method to format(in a way like strftime does)
    • I always thought it'd be good for Go to have a strftime method. This could make both happy for those who have written old(I mean, before that change) code and those who will write new(strftime-like) code.

Except for the last suggestion, those two above should be in Go 2 milestone since there should be a guarantee that it'll not break the existing code in Go 1.

And you can give a look at this code, which might be DEFINITELY not the one you were looking for: https://play.golang.org/p/3G7dNIPYdMo

@robpike
Copy link
Contributor

robpike commented Dec 28, 2017

I think it might be best to roll back for 1.10 and approach with a longer lead-up in the next release, if at all.

@hallazzang
Copy link
Contributor

Ok, that looks fine too.

@agnivade
Copy link
Contributor

agnivade commented Jan 3, 2018

Since the use of _1 is a bit obscure, my inclination is to roll back the change rather than break (admittedly obscure) existing code

It's not that obscure. This bit me today and had me debugging for quite some time till I realised this was a bug in master. I believe something like time.Now().UTC().Format("2006-01-02_15-04-05") can be pretty common.

Failing code - https://github.com/agnivade/funnel/blob/master/rollup.go#L19
Travis failure here - https://travis-ci.org/agnivade/funnel/jobs/324477279

tt := time.Now().UTC()
t.Log(tt.Format("2006-01-02_15-04-05"))
t.Log(tt.Format("2006-01-02#15-04-05"))

gives

rollup_test.go:48: 2018-01-03 127-56-27
rollup_test.go:49: 2018-01-03#07-56-27

The results are pretty drastically different. "_07" to " 127". I would recommend rollback.

@gopherbot
Copy link

Change https://golang.org/cl/85998 mentions this issue: time: revert CL 78735 (was: space padding using underscore)

@vanackere
Copy link
Contributor

Thanks for reverting, for the record I was also affected by this (format string : 20060102_150405).

@golang golang locked and limited conversation to collaborators Jan 3, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. release-blocker
Projects
None yet
Development

No branches or pull requests

8 participants