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
Implementing correct offsets for to_local #52
Conversation
# 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
Thank you for submitting this pull request. I've added some comments to your commits. I am unsure whether If Is there a way that the offset could be applied by |
# 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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 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. |
@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. |
@philr Please review it again.
Looking forward for your feedback! |
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 > 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 > 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 Support for integers in There are two ways I can think to go about testing Alternatively, you could create a subclass of |
@philr OK, I think I'm done now. I am afraid that is a pretty big change, though (considering that not only new methods are added, but also behavior of |
@philr Any news? :) |
Thank you for your work on this. I have merged the changes into master. |
@philr When will this be released? |
@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. |
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);Currently, I'm expecting principal agreement (or disagreement) with my approaches, and optimization/code quality advises.