Return local results with the offset of the local time #49

Closed
philr opened this Issue Feb 16, 2016 · 5 comments

Comments

Projects
None yet
2 participants
@philr
Member

philr commented Feb 16, 2016

As observed by @zverok on #36, since Ruby 1.9.2, it has been possible to instantiate a Time with an offset using Time.new.

The results of conversions to local time are currently returned as UTC (when using both Time and DateTime). Such results should instead be returned with the correct offset.

@philr philr added the enhancement label Feb 16, 2016

@zverok

This comment has been minimized.

Show comment
Hide comment
@zverok

zverok Mar 24, 2016

Contributor

I tried to lay my hands on it, and it seems now the discussion is required. The change is by no means cosmetic, it is pretty huge and will require subsequent API change. For example:

# current API:
t = Time.now
# => 2016-03-24 20:10:25 +0200 

tz = TZInfo::Timezone.get('Europe/Kiev')
tz.utc_to_local(t) # real t timezone is ignored and just dropped, t treated as UTC
# => 2016-03-24 22:10:25 UTC  -- no timezone info provided, but user should think of it as "+02:00"

# ...and same here, symmetrically
tz.local_to_utc(t)
# => 2016-03-24 18:10:25 UTC 

# Proposed API:
tz.convert(t) # takes into consideration t's REAL utc offset
# => 2016-03-24 20:10:25 +0200  -- and also provides meaningful result UTC offset

Considering number of gems depending on tzinfo (including THE Rails), not everyone will be happy with this change. But currently I can't see any other reasonable solution.

Maybe compatibility layer could be provided (or unhappy ones could just rely on concrete older version).

Contributor

zverok commented Mar 24, 2016

I tried to lay my hands on it, and it seems now the discussion is required. The change is by no means cosmetic, it is pretty huge and will require subsequent API change. For example:

# current API:
t = Time.now
# => 2016-03-24 20:10:25 +0200 

tz = TZInfo::Timezone.get('Europe/Kiev')
tz.utc_to_local(t) # real t timezone is ignored and just dropped, t treated as UTC
# => 2016-03-24 22:10:25 UTC  -- no timezone info provided, but user should think of it as "+02:00"

# ...and same here, symmetrically
tz.local_to_utc(t)
# => 2016-03-24 18:10:25 UTC 

# Proposed API:
tz.convert(t) # takes into consideration t's REAL utc offset
# => 2016-03-24 20:10:25 +0200  -- and also provides meaningful result UTC offset

Considering number of gems depending on tzinfo (including THE Rails), not everyone will be happy with this change. But currently I can't see any other reasonable solution.

Maybe compatibility layer could be provided (or unhappy ones could just rely on concrete older version).

@zverok

This comment has been minimized.

Show comment
Hide comment
@zverok

zverok Mar 24, 2016

Contributor

(Though, for example, sequel implements manually functionality discussed in current issue. I've tried to understand from ActiveSupport code how much it relies on current behavior... but its just too much code for my small head.)

Contributor

zverok commented Mar 24, 2016

(Though, for example, sequel implements manually functionality discussed in current issue. I've tried to understand from ActiveSupport code how much it relies on current behavior... but its just too much code for my small head.)

@zverok

This comment has been minimized.

Show comment
Hide comment
@zverok

zverok Apr 10, 2016

Contributor

Any news here?.. Maybe I should just do a pull-request with proposed changes, for evaluation?..

Contributor

zverok commented Apr 10, 2016

Any news here?.. Maybe I should just do a pull-request with proposed changes, for evaluation?..

@philr

This comment has been minimized.

Show comment
Hide comment
@philr

philr Apr 10, 2016

Member

@zverok Thank you for your comments. Sorry for not responding earlier - I've not had the time to consider this properly.

I think that the utc_to_local and local_to_utc methods should be retained. Both should continue to ignore the offset of the parameter (treating the time as either UTC or local). The result of utc_to_local should be changed to be a local time with the correct offset. This would avoid the current confusion of returning a local time that says it is UTC.

I'd add your convert method in addition to the existing methods (I might call it to_local though). A similar period_for method could probably also be added. These methods would be the equivalent of utc_to_local and period_for_utc, but start by converting the passed in time to UTC using it's declared offset.

My reason for retaining the old methods is that the local_to_utc and period_for_local methods are able to perform checks on ambiguous and non-existent local times during DST transitions. I don't think it would be possible for convert/to_local to be able to perform these checks.

I think ActiveSupport/Rails ignores the offset of the utc_to_local result, so any changes ought to be compatible. It looks like Sequel could be broken by the proposed change though.

If this ends up being an incompatible change, the major version of TZInfo will need to be incremented as per semantic versioning.

I hope to be able to work on removing the remaining Ruby 1.8 support code over the next few weeks (which may make implementing this easier). I'll then try and look into this further. In the meantime, you are welcome to submit a pull request with proposed changes and I'll take a look.

Member

philr commented Apr 10, 2016

@zverok Thank you for your comments. Sorry for not responding earlier - I've not had the time to consider this properly.

I think that the utc_to_local and local_to_utc methods should be retained. Both should continue to ignore the offset of the parameter (treating the time as either UTC or local). The result of utc_to_local should be changed to be a local time with the correct offset. This would avoid the current confusion of returning a local time that says it is UTC.

I'd add your convert method in addition to the existing methods (I might call it to_local though). A similar period_for method could probably also be added. These methods would be the equivalent of utc_to_local and period_for_utc, but start by converting the passed in time to UTC using it's declared offset.

My reason for retaining the old methods is that the local_to_utc and period_for_local methods are able to perform checks on ambiguous and non-existent local times during DST transitions. I don't think it would be possible for convert/to_local to be able to perform these checks.

I think ActiveSupport/Rails ignores the offset of the utc_to_local result, so any changes ought to be compatible. It looks like Sequel could be broken by the proposed change though.

If this ends up being an incompatible change, the major version of TZInfo will need to be incremented as per semantic versioning.

I hope to be able to work on removing the remaining Ruby 1.8 support code over the next few weeks (which may make implementing this easier). I'll then try and look into this further. In the meantime, you are welcome to submit a pull request with proposed changes and I'll take a look.

@zverok

This comment has been minimized.

Show comment
Hide comment
@zverok

zverok Apr 11, 2016

Contributor

@philr Thank you for reply and sorry for my annoyance.
Will look at it during this week.

Contributor

zverok commented Apr 11, 2016

@philr Thank you for reply and sorry for my annoyance.
Will look at it during this week.

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