Implementing correct offsets for to_local #52

Merged
merged 9 commits into from Sep 19, 2016

Conversation

Projects
None yet
3 participants
@zverok
Contributor

zverok commented May 26, 2016

Hey. This is an attempt to implement #49, as @philr described in #49 (comment).

What is done:

  • Timezone#utc_to_local returns Time/DateTime with correct offsets;
  • Timezone#local_to_utc is untouched (as discussed), it just ignores argument's offset;
  • Timezone#to_local converts argument, considering its offset.

What is not done (and requires advises/discussions):

  • Implementations are somewhat naive and maybe not the most effective, to say the least;
  • #to_local may seem undertested (and #period_for is not tested at all);
  • No documentation for new/changed functionality.

Currently, I'm expecting principal agreement (or disagreement) with my approaches, and optimization/code quality advises.

lib/tzinfo/time_or_datetime.rb
+ # so, (18:00 UTC).offset(3600) will became (18:00 +01:00).
+ # Considers original value's UTC offset wisely.
+ def offset(seconds)
+ return self if seconds == 0

This comment has been minimized.

@philr

philr May 30, 2016

Member

If the TimeOrDateTime represents a Time or DateTime with an offset, then shouldn't calling offset(0) change the offset to 0 instead of leaving it unchanged?

@philr

philr May 30, 2016

Member

If the TimeOrDateTime represents a Time or DateTime with an offset, then shouldn't calling offset(0) change the offset to 0 instead of leaving it unchanged?

lib/tzinfo/time_or_datetime.rb
+ off = OffsetRationals.rational_for_offset(seconds)
+ TimeOrDateTime.new(@orig.new_offset(off), false)
+ elsif @orig.is_a?(Time)
+ offset_str = '%+03i:%02i' % [seconds / 3600, seconds / 60 % 60]

This comment has been minimized.

@philr

philr May 30, 2016

Member

It looks like the Time constructor can take either a String or an Integer number of seconds as the offset. Would it be better to avoid the conversion to a String?

@philr

philr May 30, 2016

Member

It looks like the Time constructor can take either a String or an Integer number of seconds as the offset. Would it be better to avoid the conversion to a String?

This comment has been minimized.

@zverok

zverok May 30, 2016

Contributor

Yeah, you are right. Didn't find this on time, will change.

@zverok

zverok May 30, 2016

Contributor

Yeah, you are right. Didn't find this on time, will change.

lib/tzinfo/time_or_datetime.rb
+ TimeOrDateTime.new(time, false)
+ else
+ # Integer: fallback to "just shift timestamp"
+ TimeOrDateTime.new(@orig + seconds)

This comment has been minimized.

@philr

philr May 30, 2016

Member

This behaves slightly differently to the Time and DateTime cases, which feels a bit suspect to me.

@philr

philr May 30, 2016

Member

This behaves slightly differently to the Time and DateTime cases, which feels a bit suspect to me.

lib/tzinfo/timezone.rb
@@ -418,6 +418,21 @@ def period_for_local(local, dst = Timezone.default_dst)
end
end
+ def period_for(tm, dst = Timezone.default_dst)

This comment has been minimized.

@philr

philr May 30, 2016

Member

period_for doesn't need a dst parameter. There is no ambiguity to resolve since we know the UTC time (either directly or as a local time with an offset).

@philr

philr May 30, 2016

Member

period_for doesn't need a dst parameter. There is no ambiguity to resolve since we know the UTC time (either directly or as a local time with an offset).

This comment has been minimized.

@zverok

zverok May 30, 2016

Contributor

Yep, missed this one.

@zverok

zverok May 30, 2016

Contributor

Yep, missed this one.

lib/tzinfo/timezone.rb
+
+ utc = case tm
+ when Time
+ tm.utc

This comment has been minimized.

@philr

philr May 30, 2016

Member

Time#utc modifies the receiver (which should be avoided) instead of creating a new instance.

@philr

philr May 30, 2016

Member

Time#utc modifies the receiver (which should be avoided) instead of creating a new instance.

This comment has been minimized.

@zverok

zverok May 30, 2016

Contributor

Wow. TIL. 10 years of Ruby programming and was always sure that Ruby's Time is immutable (at least by this kind of methods). Need to do my homework better.

@zverok

zverok May 30, 2016

Contributor

Wow. TIL. 10 years of Ruby programming and was always sure that Ruby's Time is immutable (at least by this kind of methods). Need to do my homework better.

lib/tzinfo/timezone.rb
+ def period_for(tm, dst = Timezone.default_dst)
+ tm = tm.to_orig if tm.is_a?(TimeOrDateTime)
+
+ utc = case tm

This comment has been minimized.

@philr

philr May 30, 2016

Member

Can this logic to handle the different types of time representation be moved within the TimeOrDateTime class?

@philr

philr May 30, 2016

Member

Can this logic to handle the different types of time representation be moved within the TimeOrDateTime class?

@philr

This comment has been minimized.

Show comment
Hide comment
@philr

philr May 30, 2016

Member

Thank you for submitting this pull request. I've added some comments to your commits.

I am unsure whether TimeOrDateTime should be representing Time or DateTime instances that have offsets. Since only Time and DateTime can have an offset and TimeOrDateTime can also handle Integer timestamps, this feels slightly inelegant to me.

If TimeOrDateTime is going to represent Times and DateTimes with offsets, then the #to_time and #to_datetime methods should probably be changed to preserve the offset. The the #<=> method may also need to be looked at.

Is there a way that the offset could be applied by TimeOrDateTime.wrap when the result is 'unwrapped' instead? Maybe by having the wrap block return a UTC time together with the desired offset instead of the converted result.

Member

philr commented May 30, 2016

Thank you for submitting this pull request. I've added some comments to your commits.

I am unsure whether TimeOrDateTime should be representing Time or DateTime instances that have offsets. Since only Time and DateTime can have an offset and TimeOrDateTime can also handle Integer timestamps, this feels slightly inelegant to me.

If TimeOrDateTime is going to represent Times and DateTimes with offsets, then the #to_time and #to_datetime methods should probably be changed to preserve the offset. The the #<=> method may also need to be looked at.

Is there a way that the offset could be applied by TimeOrDateTime.wrap when the result is 'unwrapped' instead? Maybe by having the wrap block return a UTC time together with the desired offset instead of the converted result.

lib/tzinfo/time_or_datetime.rb
+ # preserving original class. Does NOT changes values of hour-min-second,
+ # so, (18:00 UTC).offset(3600) will became (18:00 +01:00).
+ # Considers original value's UTC offset wisely.
+ def offset(seconds)

This comment has been minimized.

@philr

philr May 30, 2016

Member

This seems to produce a different result depending on whether DateTime or Time is used to initialize the TimeOrDateTime:

TZInfo::TimeOrDateTime.new(Time.new(2016,5,30,18,0,0,'+01:00'), false).offset(3600)
 => #<TZInfo::TimeOrDateTime: 2016-05-30 19:00:00 +0100> 
TZInfo::TimeOrDateTime.new(DateTime.new(2016,5,30,18,0,0,'+01:00'), false).offset(3600)
 => #<TZInfo::TimeOrDateTime: #<DateTime: 2016-05-30T18:00:00+01:00 ((2457539j,61200s,0n),+3600s,2299161j)>>
@philr

philr May 30, 2016

Member

This seems to produce a different result depending on whether DateTime or Time is used to initialize the TimeOrDateTime:

TZInfo::TimeOrDateTime.new(Time.new(2016,5,30,18,0,0,'+01:00'), false).offset(3600)
 => #<TZInfo::TimeOrDateTime: 2016-05-30 19:00:00 +0100> 
TZInfo::TimeOrDateTime.new(DateTime.new(2016,5,30,18,0,0,'+01:00'), false).offset(3600)
 => #<TZInfo::TimeOrDateTime: #<DateTime: 2016-05-30T18:00:00+01:00 ((2457539j,61200s,0n),+3600s,2299161j)>>
@zverok

This comment has been minimized.

Show comment
Hide comment
@zverok

zverok May 31, 2016

Contributor

@philr Yeah, looking at your comments and trying to change the code accordingly I've already felt that the solution is not as clear as I've thought about it initially. Will rethink the thing at weekend.

Contributor

zverok commented May 31, 2016

@philr Yeah, looking at your comments and trying to change the code accordingly I've already felt that the solution is not as clear as I've thought about it initially. Will rethink the thing at weekend.

@zverok

This comment has been minimized.

Show comment
Hide comment
@zverok

zverok Jun 8, 2016

Contributor

@philr After considering several possibilities I should say I am still sure that this "preserve offset" staff should go into TimeOrDateTime (just being silently swallowed for integer timestamps, which, in any case, just a rarely used fallback?..)

Also, I became assured that my current implementation is really clamsy AND buggy (despite doing well on all the tests, I've just didn't add enough of them). I'll try to cleanup and fix it to the point of looking complete and compact, and will try to discuss/defend it once more.

Contributor

zverok commented Jun 8, 2016

@philr After considering several possibilities I should say I am still sure that this "preserve offset" staff should go into TimeOrDateTime (just being silently swallowed for integer timestamps, which, in any case, just a rarely used fallback?..)

Also, I became assured that my current implementation is really clamsy AND buggy (despite doing well on all the tests, I've just didn't add enough of them). I'll try to cleanup and fix it to the point of looking complete and compact, and will try to discuss/defend it once more.

@zverok

This comment has been minimized.

Show comment
Hide comment
@zverok

zverok Jun 11, 2016

Contributor

@philr Please review it again.
Highlights:

  1. I'm still sticking to the idea of TimeOrDateTime handling offsets. It doesn't always looks elegant, but if we want to preserve both "convert ignoring original offset" & "convert considering original offset" (I understand the reasoning behind this)—that's, as far as I can see, is the simplest code possible.
  2. And yes, it would behave inconsistently for integer timestamps... Which is kinda "by design" (they can't have offset information in any form)
  3. Timezone#period_for is pretty clear now. Need advice how to test it wisely.
  4. I've updated #to_time/#to_datetime in TimeOrDateTime (which, eventually, led to small method renamings and resulted in cleaner offset-handling code than in my previous commits)

Looking forward for your feedback!

Contributor

zverok commented Jun 11, 2016

@philr Please review it again.
Highlights:

  1. I'm still sticking to the idea of TimeOrDateTime handling offsets. It doesn't always looks elegant, but if we want to preserve both "convert ignoring original offset" & "convert considering original offset" (I understand the reasoning behind this)—that's, as far as I can see, is the simplest code possible.
  2. And yes, it would behave inconsistently for integer timestamps... Which is kinda "by design" (they can't have offset information in any form)
  3. Timezone#period_for is pretty clear now. Need advice how to test it wisely.
  4. I've updated #to_time/#to_datetime in TimeOrDateTime (which, eventually, led to small method renamings and resulted in cleaner offset-handling code than in my previous commits)

Looking forward for your feedback!

@philr

This comment has been minimized.

Show comment
Hide comment
@philr

philr Jul 14, 2016

Member

Thank you for updating this pull request. I'm sorry for the delay in taking a look at your latest changes. This is now looking much cleaner and more consistent.

There are still some oddities around the integer handling in TimeOrDateTime. For example, when using #to_i and #to_offset, the order matters:

> TZInfo::TimeOrDateTime.new(Time.utc(2016,7,14,12,0,0)).to_offset(3600).to_i
=> 1468497600 
> TZInfo::TimeOrDateTime.new(Time.utc(2016,7,14,12,0,0).to_i).to_offset(3600).to_i
=> 1468501200 

Comparisons can also be affected (these two TimeOrDateTime instances should really be equal):

> TZInfo::TimeOrDateTime.new(Time.utc(2016,7,14,12,0,0)).to_offset(3600) <=> 
      TZInfo::TimeOrDateTime.new(Time.utc(2016,7,14,12,0,0).to_i).to_offset(3600)
=> -1 

TZInfo does perform comparisons between Integer and Time/DateTime based TimeOrDateTime instances, but only ever in UTC. These issues therefore probably don't matter.

Support for integers in TimeOrDateTime was originally only intended for use internally within TZInfo and 'leaked' into the public interface. I doubt that there is any significant use of this feature. I'll have a think about dropping support for integers in the public interface and removing support for integer representations in TimeOrDateTime as a separate change.

There are two ways I can think to go about testing Timezone#period_for. You could identify some time periods in one of the included test time zones (see the test/zoneinfo or test/tzinfo-data/tzinfo directories) where the offset will tip the result one way or the other and check the result.

Alternatively, you could create a subclass of Timezone for testing that overrides #period_for_utc and verify that #period_for invokes #period_for_utc with the expected parameter.

Member

philr commented Jul 14, 2016

Thank you for updating this pull request. I'm sorry for the delay in taking a look at your latest changes. This is now looking much cleaner and more consistent.

There are still some oddities around the integer handling in TimeOrDateTime. For example, when using #to_i and #to_offset, the order matters:

> TZInfo::TimeOrDateTime.new(Time.utc(2016,7,14,12,0,0)).to_offset(3600).to_i
=> 1468497600 
> TZInfo::TimeOrDateTime.new(Time.utc(2016,7,14,12,0,0).to_i).to_offset(3600).to_i
=> 1468501200 

Comparisons can also be affected (these two TimeOrDateTime instances should really be equal):

> TZInfo::TimeOrDateTime.new(Time.utc(2016,7,14,12,0,0)).to_offset(3600) <=> 
      TZInfo::TimeOrDateTime.new(Time.utc(2016,7,14,12,0,0).to_i).to_offset(3600)
=> -1 

TZInfo does perform comparisons between Integer and Time/DateTime based TimeOrDateTime instances, but only ever in UTC. These issues therefore probably don't matter.

Support for integers in TimeOrDateTime was originally only intended for use internally within TZInfo and 'leaked' into the public interface. I doubt that there is any significant use of this feature. I'll have a think about dropping support for integers in the public interface and removing support for integer representations in TimeOrDateTime as a separate change.

There are two ways I can think to go about testing Timezone#period_for. You could identify some time periods in one of the included test time zones (see the test/zoneinfo or test/tzinfo-data/tzinfo directories) where the offset will tip the result one way or the other and check the result.

Alternatively, you could create a subclass of Timezone for testing that overrides #period_for_utc and verify that #period_for invokes #period_for_utc with the expected parameter.

@zverok

This comment has been minimized.

Show comment
Hide comment
@zverok

zverok Aug 7, 2016

Contributor

@philr OK, I think I'm done now. period_for is tested properly, all tests are passing, and documentation for new methods are added.

I am afraid that is a pretty big change, though (considering that not only new methods are added, but also behavior of utc_to_local was changed).

Contributor

zverok commented Aug 7, 2016

@philr OK, I think I'm done now. period_for is tested properly, all tests are passing, and documentation for new methods are added.

I am afraid that is a pretty big change, though (considering that not only new methods are added, but also behavior of utc_to_local was changed).

@zverok zverok changed the title from [WIP] Implementing correct offsets for to_local to Implementing correct offsets for to_local Aug 14, 2016

@zverok

This comment has been minimized.

Show comment
Hide comment
@zverok

zverok Sep 2, 2016

Contributor

@philr Any news? :)

Contributor

zverok commented Sep 2, 2016

@philr Any news? :)

@philr philr merged commit 85ffbda into tzinfo:master Sep 19, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

philr added a commit that referenced this pull request Sep 19, 2016

@philr

This comment has been minimized.

Show comment
Hide comment
@philr

philr Sep 19, 2016

Member

Thank you for your work on this. I have merged the changes into master.

Member

philr commented Sep 19, 2016

Thank you for your work on this. I have merged the changes into master.

@conorliv

This comment has been minimized.

Show comment
Hide comment
@conorliv

conorliv Oct 21, 2016

@philr When will this be released?

@philr When will this be released?

@philr

This comment has been minimized.

Show comment
Hide comment
@philr

philr Oct 21, 2016

Member

@conorliv I'm currently working on making some changes to other places that return local times so that they all consistently return an offset. I'm also doing some tidying up following increasing the minimum Ruby version to 1.9.3.

Once all of that is out of the way, I'll probably look at creating a new release. I can't make any promises as to when that will be though.

Member

philr commented Oct 21, 2016

@conorliv I'm currently working on making some changes to other places that return local times so that they all consistently return an offset. I'm also doing some tidying up following increasing the minimum Ruby version to 1.9.3.

Once all of that is out of the way, I'll probably look at creating a new release. I can't make any promises as to when that will be though.

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