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

Add ToTimeInDefaultLocation/E #23

Closed
wants to merge 1 commit into from
Closed

Conversation

heewa
Copy link
Contributor

@heewa heewa commented Mar 16, 2016

Go's time parsing uses UTC when the format doesn't have a timezone, and has even weirder behavior when it has a zone name but no numeric offset. A caller to cast.ToTime cannot know if the returned time was explicitly in UTC or was set by default, so they cannot fix it. These new functions allow a user to supply a different timezone to default to, with nil using the local zone.

This addresses gohugoio/hugo#1882 by allowing that code to use cast.ToTimeInDefaultLocation(raw, nil).

A note on the implementation: It's not possible to use time.ParseInLocation because parsing with a format that has a named zone but no numeric offset (such as time.RC1123Z) returns a time with a 0 offset in that name iff that's not the current local timezone (!!). You can verify this by changing like 521 to use time.ParseInLocation(format.format, s, defaultLocation) and commenting out the manual override in the body of the if clause – tests will fail.

I think the root cause is that the time pkg has two parsing functions: time.Parse and time.ParseInLocation, both of which use a private workhorse fn time.parse. That private fn takes two zones: a default and what's considered "local". I'm not sure, but I think in order to get the effect we want, we'd need to call that fn with our default and the current local zone. However, time.Parse uses default of UTC, whereas time.ParseInLocation uses the given zone for both default & local.

Go's time parsing uses UTC when the format doesn't have a tiemzone, and
has even weirder behavior when it has a zone name but no numeric offset.
A caller to `cast.ToTime` won't know if the returned time was explicitly
in UTC, or defaulted there, so the caller cannot fix it. These new
functions allow a user to supply a different timezone to default to,
with nil using the local zone.
@CLAassistant
Copy link

CLAassistant commented Mar 16, 2016

CLA assistant check
All committers have signed the CLA.

@bep
Copy link
Collaborator

bep commented Mar 16, 2016

Thanks for this; I'll have reread and try to understand this. Seems a little bit weird from the std lib ...

@heewa
Copy link
Contributor Author

heewa commented Mar 16, 2016

:) Here's a quick playground to illustrate: https://play.golang.org/p/UqNajdLppw

I'd love a simpler, smaller, cleaner solution. Let me know if you can think of one. Another approach, though it's arguably no simpler, would be to tag formats that have only a named zone & no offset, then use time.ParseInLocation and only manually convert the tagged formats. That way the manual override happens on a smaller set of formats.

@bep
Copy link
Collaborator

bep commented Apr 8, 2016

I haven't forgotten you. I will try to wrap my head around this tomorrow. Your workaround looks fine, but I still think this looks like a flaw in Go's stdlib (which means we should provide this as a temp workaround and hope for a fix ...), but time and timezones are some of the harder problems of computer science ...

@bep
Copy link
Collaborator

bep commented Sep 19, 2016

I haven't forgotten, but busy ... @spf13 any opinion on this?

@bep
Copy link
Collaborator

bep commented May 30, 2019

@heewa I'm really sorry to have left this hanging here for several years (time flies!). I have spent some time trying to understand the rather confusing time.Parse API ... And I think I now kind of understand it.

Let us finish this in #79

@bep bep closed this May 30, 2019
@bep bep mentioned this pull request Jun 1, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants