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

Timezone support by Time in Ruby 2.6 #94

Closed
nobu opened this Issue Sep 27, 2018 · 4 comments

Comments

Projects
None yet
3 participants
@nobu
Copy link
Contributor

nobu commented Sep 27, 2018

Built-in Time class in Ruby 2.6 is going to support timezone, as this ticket.
My patch works fine with tzinfo 1.2.2, but not with 2.0.0.pre1, because TZInfo::Timestamp.for dispatches by its argument's class.
Couldn't you consider to accept a Time-like object which have to_i method (and maybe subsec)?

Thank you.

philr added a commit that referenced this issue Oct 7, 2018

@philr

This comment has been minimized.

Copy link
Member

philr commented Oct 7, 2018

Sorry for the delay in responding to this.

I see that you've now changed Time::TM to be a subclass of Time. I assume that this is a temporary workaround and not intended to be retained?

I've pushed a new feature/support_time_tm_struct branch with changes to TZInfo::Timestamp to handle the Struct version of Time::TM. I've kept the argument class checking. I prefer this approach because Timestamp.for also converts the block result back to the type of the given value.

Note that TZInfo 1.2 is also checking classes, but falls back to calling #to_i. This will cause any sub-second precision to be lost and make methods like utc_to_local and local_to_utc return an Integer instead of a Time.

@nobu

This comment has been minimized.

Copy link
Contributor Author

nobu commented Oct 15, 2018

Regarding the superclass of Time::TM, I'm still wondering what it should be.
@akr, do you have any opinion?

In general, I don't think checking classes is preferred in ruby.
Falling back to calling #to_i (and #subsec optionally) feels better, or coercing by #to_time.

@akr

This comment has been minimized.

Copy link

akr commented Oct 15, 2018

Time::TM is a internal mechanism of Time.
So, "Time::TM inherits Time" means "Time use Time internally".
I think it is needlessly complex.
So, I vote Time::TM doesn't inherit Time.

@philr philr closed this in 32e65c2 Nov 11, 2018

@philr

This comment has been minimized.

Copy link
Member

philr commented Nov 11, 2018

I've committed a change to master to accept Time-like values (checking that the object responds to both to_i and subsec).

According to https://ruby-doc.org/core-2.6.0.preview3/Time.html#class-Time-label-Timezone+argument, 'the sub-second attributes [of the Time-like object] are fixed as 0', so TZInfo 1.2 only calling #to_i should not be an issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.