proposal: time: Add function to generate a Location struct

[FlorianUekermann]

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.

[bradfitz]

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]

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]

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]

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]

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]

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]

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]

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]

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

[dmitshur]

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]

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]

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]

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]

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

[gopherbot]

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

[FlorianUekermann]

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

[rsc]

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]

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]

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]

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

[slotheroo]

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.