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

Use T.any(Integer, Float, BigDecimal) as the return type for ActiveRecord calculation methods #1879

Merged

Conversation

olivier-thatch
Copy link
Contributor

@olivier-thatch olivier-thatch commented Apr 22, 2024

Use T.any(Integer, Float, BigDecimal) as the return type for ActiveRecord calculation methods.

Closes #1850.

Motivation

See discussion in #1850.

Implementation

Updated ActiveRecordRelations compiler to use T.any(Integer, Float, BigDecimal) instead of Numeric as the return type for sum methods.

Some notes/thoughts:

  • Should other calculation methods be updated accordingly? E.g. average, minimum, maximum, calculate
    • Yes ➡️ Done!
  • BigDecimal is only available after require "bigdecimal" (and, in a future Ruby version, bigdecimal will switch from a default gem to a bundled gem). That's probably fine since bigdecimal is a dependency of activesupport, so presumably any Rails / ActiveRecord user already has the gem in their app.

Tests

Updated existing tests in spec/tapioca/dsl/compilers/active_record_relations_spec.rb

@egiurleo
Copy link
Contributor

I'm fine keeping BigDecimal for now because most Rails users will have it loaded. We can improve that if somebody opens an issue about it!

I think we should update the other calculation methods so the entire RBI file is consistent. I imagine it would be confusing for someone to run into the issues you've been having (having to cast all the time) but only for certain methods.

@olivier-thatch olivier-thatch force-pushed the olivier/improve-sum-return-type branch from 8d9b44e to b548a64 Compare April 23, 2024 15:58
@olivier-thatch olivier-thatch changed the title Use T.any(Integer, Float, BigDecimal) as the return type for sum methods Use T.any(Integer, Float, BigDecimal) as the return type for ActiveRecord calculation methods Apr 23, 2024
@olivier-thatch
Copy link
Contributor Author

@egiurleo I've updated the PR to use T.any(Integer, Float, BigDecimal) for all ActiveRecord calculation methods.

@egiurleo egiurleo merged commit 7533e71 into Shopify:main Apr 23, 2024
18 checks passed
@olivier-thatch olivier-thatch deleted the olivier/improve-sum-return-type branch April 23, 2024 20:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Consider not using Numeric for ActiveRecord #sum
2 participants