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

Support timezone object argument to Time.{at,new} and Time#{getlocal,localtime} #3740

Open
wants to merge 22 commits into
base: master
Choose a base branch
from

Conversation

rwstauner
Copy link
Collaborator

This adds (at least some) support for the "Timezone Objects" interfaces to several Time methods as described in
https://docs.ruby-lang.org/en/3.3/Time.html#class-Time-label-Timezone+Objects

It was difficult to discern how it all should work but between the CRuby source code and the specs (and tracing what methods were being called on our objects) I think we figured it out.

We started this six months ago as part of HackDays and didn't get very far, then I resurrected it this week.

closes #1717

@oracle-contributor-agreement oracle-contributor-agreement bot added the OCA Verified All contributors have signed the Oracle Contributor Agreement. label Dec 13, 2024
@andrykonchin
Copy link
Member

Thank you! Will review the PR at this a bit later.

@andrykonchin
Copy link
Member

andrykonchin commented Dec 24, 2024

Wondering what logic is behind the utc_offset.nonzero? check:

offset = as_local.utc_offset.nonzero? || as_local.to_i - time.to_i

It seems to me it should use either #utc_offset or difference between #to_i values. But it seems not so simple.

@andrykonchin andrykonchin added the in-ci The PR is being tested in CI. Do not push new commits. label Dec 24, 2024
@rwstauner
Copy link
Collaborator Author

Wondering what logic is behind the utc_offset.nonzero? check:

offset = as_local.utc_offset.nonzero? || as_local.to_i - time.to_i

It seems to me it should use either #utc_offset or difference between #to_i values. But it seems not so simple.

Originally I seemed to need it... I had to fight a bit with the implementation to make the specs pass because of this trick that happens in the specs:

https://github.com/oracle/truffleruby/blob/9f6f6a7ae38c62bb808c48b0b7560b3fb47f980f/./spec/ruby/core/time/fixtures/classes.rb#L18

Probably once the implementation was right it wasn't needed any more.

The whole thing was a bit confusing. We discovered that in CRuby there's a whole hidden class that gets used for this purpose (Time::tm) which is similar to Time but not quite the same.

Thanks for investigating this and improving it!

rwstauner and others added 20 commits February 6, 2025 20:10
Expected TypeError ((?-mix:can't convert \w+ into an exact number))
but got: TypeError (wrong argument type Object (expected Integer))
> Expected TypeError ((?-mix:can't convert \w+ into an exact number))
> but got: TypeError (wrong argument type Object (expected Integer))

close enough
In CRuby this spec passes because it is a Time-Like object (Time::tm)
that defines to_time.
We are using an actual Time object here so we need the method
for the spec to pass.
…stance of Time and take into account #utc_offset
@andrykonchin andrykonchin force-pushed the rwstauner/timezone-arg branch from 9f6f6a7 to f4eea14 Compare February 7, 2025 14:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in-ci The PR is being tested in CI. Do not push new commits. OCA Verified All contributors have signed the Oracle Contributor Agreement. shopify
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Time.new and Time#getlocal don't accept a timezone object or a UTC offset string
4 participants