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

Return TimeWithZone from Time.current #262

Closed
wants to merge 1 commit into from

Conversation

ipvalverde
Copy link
Contributor

@ipvalverde ipvalverde commented Jul 15, 2024

Type of Change

  • Add RBI for a new gem
  • Modify RBI for an existing gem
  • Other:

Changes

  • Gem name: activesupport
  • Gem version:
  • Gem source:
  • Gem API doc: Time.current and Time.zone
  • Tapioca version:
  • Sorbet version:

Description

Fixes #261

Although the documentation does indicate that Time.current should be able to return a Time object this is unlikely to happen in a Rails application. This PR is to mitigate the need of extra code from consumers that might want to assign the return of Time.current to a TimeWithZone.

Ideally we should make the TimeWithZone to inherit from Time, since it hacks its way to be like a Time, but this was failing CI and it can be in a different PR.

@ipvalverde ipvalverde changed the title Return TimeWIthZone from Time.current Return TimeWithZone from Time.current Jul 16, 2024
@ipvalverde ipvalverde marked this pull request as ready for review July 16, 2024 13:10
@ipvalverde ipvalverde requested a review from a team as a code owner July 16, 2024 13:10
@paracycle
Copy link
Member

Active Support is not only used in Rails applications, so making the return type fit just a majority of Rails applications is problematic in my opinion.

Since this is dynamic and the return value depends on the presence of Time.zone, I'd rather that we create a DSL compiler for this in Tapioca which would create the right signature for the application it is working against.

@ipvalverde ipvalverde force-pushed the iv/make-time-extend-time-with-zone branch from f0734fc to 689baf8 Compare July 17, 2024 10:42
@ipvalverde ipvalverde marked this pull request as draft July 17, 2024 10:44
@paracycle
Copy link
Member

I just created a PR for the compiler, btw: Shopify/tapioca#1962

@amomchilov
Copy link
Contributor

Shall we close this draft then?

@amomchilov amomchilov closed this Aug 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Updated Time annotations breaks model.updated_at = Time.current
3 participants