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 enumeration for locations #36278

Closed
bakurits opened this issue Dec 25, 2019 · 9 comments
Closed

proposal: time: add enumeration for locations #36278

bakurits opened this issue Dec 25, 2019 · 9 comments
Labels
Milestone

Comments

@bakurits
Copy link

@bakurits bakurits commented Dec 25, 2019

I think it would be nice to have an enumeration for IANA Time Zone database names instead of strings

@gopherbot gopherbot added this to the Proposal milestone Dec 25, 2019
@gopherbot gopherbot added the Proposal label Dec 25, 2019
@smasher164 smasher164 changed the title proposal: add enumeration for locations in the time package proposal: time: add enumeration for locations Dec 25, 2019
@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Dec 26, 2019

Please give some examples of what you would like to see. Thanks.

As far as I can see the list of IANA timezone names is quite long and subject to regular change: https://en.wikipedia.org/wiki/List_of_tz_database_time_zones .

@bakurits
Copy link
Author

@bakurits bakurits commented Dec 26, 2019

loc, err := time.LoadLocation(time.Asia_Shanghai)
Something like that.

Does the location names change frequently? If that's the case I see the problem

@xhit
Copy link
Contributor

@xhit xhit commented Dec 26, 2019

The timezone database updates frequently. To update timezone database the Go team can release it with each Go version or create a mechanism to update it with go get (better option I think)

@ALTree
Copy link
Member

@ALTree ALTree commented Dec 26, 2019

But why is LoadLocation(time.Asia_Shanghai) better than LoadLocation("Asia/Shanghai") ?

@bakurits
Copy link
Author

@bakurits bakurits commented Dec 26, 2019

But why is LoadLocation(time.Asia_Shanghai) better than LoadLocation("Asia/Shanghai") ?

Because of autocompletion. In first approach there will be compile time error and in the second one there will be error in run time

@ALTree
Copy link
Member

@ALTree ALTree commented Dec 26, 2019

The problem is that we read timezones data from the system database, and we support the timezones names and abbreviations we read from the system... So if, for example, we include Asia_Shanghai in the enum but then Asia/Shanghai is not in the timezones db the call will fail at runtime anyway.

And what happens if a name is removed from the DB? We can't remove it from the enum without breaking backward compatibility.

@ALTree
Copy link
Member

@ALTree ALTree commented Dec 26, 2019

My point is that it's probably not worth having compile-time checks (in the form of a fixed enum) on something that is gonna be dynamic anyway like timezones data (which is dynamic because as I wrote we read the data at runtime from the system database).

@bakurits
Copy link
Author

@bakurits bakurits commented Dec 26, 2019

My point is that it's probably not worth having compile-time checks (in the form of a fixed enum) on something that is gonna be dynamic anyway like timezones data (which is dynamic because as I wrote we read the data from the system database, at runtime).

Yes, you are right, there are tradeoffs in both side, I just added this issue for discussion, I wanted to know other people's opinion about what was better

@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Dec 27, 2019

Since we get the timezone information from the local system, I agree that it would be unfortunate to define enum values for timezones that are not necessarily supported.

@bakurits bakurits closed this Dec 27, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
5 participants
You can’t perform that action at this time.