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

Comments

Projects
None yet
8 participants
@fsouza
Copy link
Contributor

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

This comment has been minimized.

Copy link
Contributor

commented Dec 27, 2017

Git bisect points to 8776be1

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

commented Dec 27, 2017

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

@hallazzang

This comment has been minimized.

Copy link
Contributor

commented Dec 28, 2017

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

This comment has been minimized.

Copy link
Contributor

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

This comment has been minimized.

Copy link
Contributor

commented Dec 28, 2017

Ok, that looks fine too.

@agnivade

This comment has been minimized.

Copy link
Member

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

This comment has been minimized.

Copy link

commented Jan 3, 2018

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

@gopherbot gopherbot closed this in 86cca11 Jan 3, 2018

@vanackere

This comment has been minimized.

Copy link
Contributor

commented Jan 3, 2018

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.
You can’t perform that action at this time.