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

Getting timezone identifiers from time objects #90

Closed
timcraft opened this Issue Aug 8, 2018 · 2 comments

Comments

Projects
None yet
2 participants
@timcraft

timcraft commented Aug 8, 2018

Thanks for developing and maintaining this useful gem!

I've been testing out 2.0.0.pre1 and looking into using it to eliminate some other timezone abstractions and dependencies in various projects. The blocker I keep hitting is that there's no way to get the timezone from a time, either the object or its identifier which is what I really want. For example:

require "tzinfo"

zone = TZInfo::Timezone.get("America/New_York")

time = zone.local_time(2015, 2, 24, 10, 0, 0)

time.zone  # => "EST"

# How to get "America/New_York" from time, without a reference to zone?

Similar situation to what was discussed briefly in #86 which I'm assuming is because Time#zone is already well defined and needing to be compatible with that. A few options I can think of:

  • Add a #zone_identifier method (clear and simple, if a little wordy).

  • Add a #tz or #timezone method to return the timezone object (a bit more flexible, but calling time.timezone doesn't seem very elegant).

  • Add something more elaborate which would support time.zone.identifier as well as time.zone acting like a String for semantic compatibility with Time#zone.

Would any of these be acceptable, or any other solutions that would be preferable?

@philr

This comment has been minimized.

Member

philr commented Aug 8, 2018

The current implementation on master returns extended TimeWithOffset, DateTimeWithOffset or TimestampWithOffset instances to represent local results. Each of these is associated with the TimezoneOffset in force at the time.

I have considered including a reference to the Timezone or identifier in the extended local Time and DateTime results. I decided against doing this for two reasons:

Firstly, there are places that construct extended local times that don't have any reference to the Timezone (TimezonePeriod#local_starts_at, TimezonePeriod#local_ends_at, TimezoneTransition#local_end_at and TimezoneTransition#local_start_at). I don't think it would be right to have some places returning the associated time zone and others not. Making the Timezone available everywhere would likely require wrapping TimezonePeriod and TimezoneTransition instances when returned by Timezone instance methods. In my mind at least, this gets a bit messy.

The second reason is that, for me, just adding the associated Timezone or identifier is not enough. I would expect the extended local Time or DateTime implementation to also respect the time zone rules when performing arithmetic. For example, this would require making DateTime#next_day add either 23, 24 or 25 hours depending on whether a daylight savings transition occurred (with options to handle ambiguity when the clock goes back and non-existent local times when the clock goes forward). Such problems have also already been solved elsewhere (for example, in ActiveSupport::TimeWithZone).

My current plan is to release 2.0.0 with TimeWithOffset and DateTimeWithOffset as they are (I don't want to make any more major changes at this point). After the release, I'll look to revisit this. My current thinking is for a separate TimeWithZone class that implements a Time-like interface, but has it's own methods for performing arithmetic, advancing across days, etc. This would not be returned by Timezone, but could be constructed separately, e.g.:

TZInfo::TimeWithZone.new('America/New_York', 2015, 2, 24, 10, 0, 0)

TZInfo::TimeWithZone.new('America/New_York, Time.now.utc)
@timcraft

This comment has been minimized.

timcraft commented Aug 9, 2018

That makes sense, thanks for the detailed explanation.

Agree it could get messy with all those other classes involved. Appreciate wanting to implement it more thoroughly, my use case is simpler because I don't need the arithmetic.

It's ActiveSupport::TimeWithZone which I'm using at the moment and looking to eliminate, tzinfo is already a transitive dependency so I'd rather use that directly if something similar to TimeWithZone could be added. It should be reasonably straightforward to implement a wrapper class without the arithmetic so that might be a suitable stopgap in the meantime.

Looking forward to the 2.0.0 release.

@timcraft timcraft closed this Aug 9, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment