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 16 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

rwstauner and others added 9 commits December 12, 2024 17:02
…localtime}

closes oracle#1717

For compatibility with https://bugs.ruby-lang.org/issues/14850

Co-authored-by: Manef Zahra <manef.zahra@shopify.com>
Co-authored-by: Patrick Lin <patrick.lin@shopify.com>
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.
@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
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
3 participants