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

proposal: time: Add function to generate a Location struct #20629

Closed
FlorianUekermann opened this issue Jun 9, 2017 · 21 comments
Closed

proposal: time: Add function to generate a Location struct #20629

FlorianUekermann opened this issue Jun 9, 2017 · 21 comments
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done. Proposal Proposal-Accepted
Milestone

Comments

@FlorianUekermann
Copy link
Contributor

If I am not mistaken it is currently not possible (without using unsafe) to generate a time.Location. This is a bit of a problem because whether LoadLocation works is quite pretty platform/instance dependent. A simple solution could be to add:
func NewLocation(name string, zone []Zone, tx []ZoneTrans) *Location
which would require making Zone(Trans)? and theyr fields public.

I guess Zone and ZoneTrans could also go into a subpackage to avoid cluttering the documentation of the time package.

@gopherbot gopherbot added this to the Proposal milestone Jun 9, 2017
@bradfitz
Copy link
Contributor

bradfitz commented Jun 9, 2017

If I am not mistaken it is currently not possible (without using unsafe) to generate a time.Location.

You're jumping to a solution here.

This is a bit of a problem because whether LoadLocation works is quite pretty platform/instance dependent

Is that the actual problem? If so, we can discuss that problem.

I guess Zone and ZoneTrans could also go into a subpackage to avoid cluttering the documentation of the time package.

We definitely don't want any types or zone constructor functions in the time package.

If we're going to add a new package, I'd rather see something like "time/zones" and then zones.Load(zoneName), where the "time/zones" package documents that has all the tzdata available (perhaps we bundle the tzdata zip file into the archive file; we still don't have an efficient way to do that without duplicating the content into a Go string file)

In any case, I'm against your proposed solution. But maybe you want to restate the problem. (Please don't edit the top comment, though.)

@FlorianUekermann
Copy link
Contributor Author

Not surprised, I am not too happy about the proposal either. I'm thinking about portability and custom locations.

If I understand the current situation correctly (not 100% sure about that after your reply), the return values of LoadLocation depend on the local zonefile, which may be outdated for example.
Making your own locations may sound silly, but it would allow shipping locations without depending on a particular environment. Another problematic case is GopherJS as platform, which doesn't support any locations (I do realize that this may not be the place to fix that though).

@bradfitz
Copy link
Contributor

bradfitz commented Jun 9, 2017

I'm thinking about portability and custom locations.

What do you mean by portability? And what is a custom location? Is there missing data in tzdata?

Another problematic case is GopherJS as platform, which doesn't support any locations

What is the GopherJS bug URL for this issue? I'm curious what the options to address it are. Presumably they can just bundle the whole tzdata zip file into the Javascript if they really wanted everything. But perhaps that's too big.

@FlorianUekermann
Copy link
Contributor Author

FlorianUekermann commented Jun 9, 2017

By portability I mean the possibility of writing code that involves time.Location instances and produces the same results independent of environment details (like OS, version/presence of tzdata).
By custom location I mean a developer defined instance of time.Location (either to ensure portability, or because a not/recently standardized timezone combination is needed). I am not sure if there is missing data in tzdata beyond the outdated version issue.

Note: Countries/states change daylight saving details quite quickly sometimes. So tzdata being out of date is not as weird as it may sound.

The GopherJS issue is gopherjs/gopherjs#64. Yes, size is a problem that is interesting. A size efficient solution that was discussed years ago is using momentsJS as a source of location data.

@rsc
Copy link
Contributor

rsc commented Jun 12, 2017

We definitely don't want to expose Zone, ZoneTrans, and so on. Location is where the API stops.

I do understand wanting to get the location info from somewhere else (a []byte in tzdata format), but the solution would be an API solving that problem.

@rsc
Copy link
Contributor

rsc commented Jun 12, 2017

Note that any solution here should probably address #20211 as well (closed as dup of this one). The theory is that if you can override where location data comes from, the override can provide its own listing function separate from package time.

@FlorianUekermann
Copy link
Contributor Author

FlorianUekermann commented Jun 13, 2017

That sounds a lot better. So I guess a solution to both issues could look like this?
NewLocation(name string, tzdata []byte) (*Location, error)
I'm not sure if that is a good name or if it should explicitly state the input format (maybe "LoadTzif"), which would also come in handy if we ever want to add other formats that may not have magic bytes.

Looking at the code this should be a tiny patch. I'm happy to submit some code if everyone is happy with this.

Btw.: What's the deal with not importing os (mentioned in #20211)? Is that a general rule?

@dmitshur
Copy link
Contributor

Btw.: What's the deal with not importing os (mentioned in #20211)? Is that a general rule?

Packages in standard library have tiers, and higher level tiers are not allowed to import lower level ones. This is to prevent import cycles, and per policy, to keep package size smaller.

See here for full details.

@rsc
Copy link
Contributor

rsc commented Jun 19, 2017

We're not sure what the API here should be. Does anyone want to make any suggestions?

@dmitshur
Copy link
Contributor

dmitshur commented Jun 19, 2017

Looking for clarification/confirmation, but what would be the uses/users of this new API? Is it needed by anyone/anything other than to resolve the underlying mentioned GopherJS issue?

Because I'm quite confident the GopherJS issue could be resolved by modifying the time package internally in its overrides, without having to add to upstream time API. So I want to confirm this is a generally useful change in the Go standard library that warrants increasing API size.

@FlorianUekermann
Copy link
Contributor Author

What about the scenarios outlined above?
The gopherjs issue just highlights the problems you can run into with LoadLocation. I definitely would like to define my own Locations as well as ensure that the locations I use are available independent of the target environment, which is not an issue that is exclusive to gopherjs.
See issue #20211 for another usecase.

@rsc
Copy link
Contributor

rsc commented Aug 14, 2017

NewLocation(name string, tzdata []byte) (*Location, error)

The signature looks right, but the name is probably not specific enough to the problem being solved. It's nice if this begins with Load to match LoadLocation. LoadTZData? LoadZoneData?

@FlorianUekermann
Copy link
Contributor Author

Yes, I would even suggest keeping both words in LoadLocation. How about LoadLocationFromTZData or LoadLocationTZData?
(I won't be able to submit code for review until the end of next week, so if anyone else has the time to do this please go ahead.)

@rsc
Copy link
Contributor

rsc commented Aug 15, 2017

OK, let's run with LoadLocationFromTZData for now. Thanks.

@rsc rsc added NeedsFix The path to resolution is known, but the work has not been done. Proposal-Accepted labels Aug 15, 2017
@gopherbot
Copy link

Change https://golang.org/cl/68890 mentions this issue: time: enable Location loading from user provided timezone data

@FlorianUekermann
Copy link
Contributor Author

I think we should call it LoadLocationFromTzinfo, because TZData sounds like the file used on Android.

@rsc
Copy link
Contributor

rsc commented Oct 9, 2017

I think we should stick with tzdata. www.google.com/search?q=tzdata vs www.google.com/search?q=tzinfo seems to show clearly that tzdata is the much more common term. (tzinfo seems to be very popular in ruby?)

I don't think enough people know about this detail of Android to cause serious confusion.

@FlorianUekermann
Copy link
Contributor Author

I don't want to go overboard with the bikeshedding, but since we'll be stuck with that name let me add this one comment before I resend the CL.

...tzdata is the much more common term.

tzdata is the common term for the whole database/archive. Android is one example.
http://www.iana.org/time-zones also uses the tzdata name for the archive that contains all files (latest version is tzdata2017b.tar.gz).

https://www.ietf.org/timezones/tzdb/tzfile.5.txt says this about the format:

TZFILE(5)                     File Formats Manual                    TZFILE(5)

NAME
       tzfile - time zone information

DESCRIPTION
       The time zone information files used by tzset(3) begin with the magic
       characters "TZif" to identify them as time zone information files,

tzinfo was probably a bad idea, but maybe the above gives someone an idea for a name that makes clear that this is a function expects the contents of one of those files, not the whole archive/database. Maybe tzfile is a better idea, since that is what the man page above and the code repo from http://www.iana.org/time-zones use.

If you want to go for tzdata, please tell me if you prefer ...FromTzdata or ...FromTZData? (Ian voiced a small preference for the ...FromTzdata capitalization in a previous CL concerning internal function names).

@ianlancetaylor
Copy link
Contributor

But nobody really says tzfile either. I suppose it could LoadLocationFromTzdataFile (or LoadLocationFromTZDataFile).

My comment about the capitalization was about the use of TZinfo when the code was already using Tzdata in various places. But that was just internal names. I'm fine with using TZData in the external name.

@gopherbot
Copy link

Change https://golang.org/cl/79176 mentions this issue: time: rename TestLoadLocationFromTzinfo to TestLoadLocationFromTZData

gopherbot pushed a commit that referenced this issue Nov 21, 2017
Tzinfo was replaced with TZData during the review of CL 68890, but this
instance was forgotten. Update it for consistency.

Follows CL 68890.
Updates #20629.

Change-Id: Id6d3c4f5f7572b01065f2db556db605452d1b570
Reviewed-on: https://go-review.googlesource.com/79176
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
@slotheroo
Copy link

Since the implementation of this issue is supposed to address #20211, I think it would make sense to make loadTzinfoFromZip an exported method as it is nontrivial to implement if someone wants to use a specific zoneinfo.zip file as their source of truth for time zones. The alternative seems to be to unzip the zoneinfo file and use a directory instead of a zip, which makes reading the correct location file much easier. Either way, the path to using a specific version of the IANA time zone database reliably is still far from clear in my opinion.

@golang golang locked and limited conversation to collaborators Jul 3, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done. Proposal Proposal-Accepted
Projects
None yet
Development

No branches or pull requests

7 participants