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

Add annotations for shopify-money gem #153

Merged
merged 4 commits into from
Apr 6, 2023

Conversation

olivier-thatch
Copy link
Contributor

Type of Change

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

Changes

  • Gem name: shopify-money
  • Gem version: 1.1.2
  • Gem source: https://github.com/Shopify/money
  • Gem API doc:
  • Tapioca version: Tapioca v0.10.5
  • Sorbet version: Sorbet typechecker 0.5.10601 git 09bae4a8a0da2140f12443bc32a2725efc738eb7 debug_symbols=true clean=1

@olivier-thatch olivier-thatch requested a review from a team as a code owner March 9, 2023 22:30
Comment on lines +13 to +14
sig { params(numeric: Numeric).returns(T.noreturn) }
def /(numeric); end
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment on lines +43 to +47
sig { params(num: Numeric).returns(T::Array[Money]) }
def split(num); end

sig { params(num: Numeric).returns(T::Hash[Money, Numeric]) }
def calculate_splits(num); end
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It doesn't really make sense to pass non-integers to the split and calculate_splits methods IMO, but technically the gem does support it (even though it produces surprising/arguably incorrect results with non-integers).

Copy link
Member

@paracycle paracycle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks this is great! But are we missing some important methods?

rbi/annotations/shopify-money.rbi Show resolved Hide resolved
Comment on lines +7 to +8
sig { returns(T.any(Money::Currency, Money::NullCurrency)) }
attr_reader :currency
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Annoyingly, Money::NullCurrency is not a subclass of Money::Currency, so we have to use a union.

Comment on lines +10 to +36
# @method_missing: delegated to BigDecimal
sig { params(args: T.untyped, _arg1: T.untyped, block: T.nilable(T.proc.void)).returns(T::Boolean) }
def zero?(*args, **_arg1, &block); end

# @method_missing: delegated to BigDecimal
sig { params(args: T.untyped, _arg1: T.untyped, block: T.nilable(T.proc.void)).returns(T::Boolean) }
def nonzero?(*args, **_arg1, &block); end

# @method_missing: delegated to BigDecimal
sig { params(args: T.untyped, _arg1: T.untyped, block: T.nilable(T.proc.void)).returns(T::Boolean) }
def positive?(*args, **_arg1, &block); end

# @method_missing: delegated to BigDecimal
sig { params(args: T.untyped, _arg1: T.untyped, block: T.nilable(T.proc.void)).returns(T::Boolean) }
def negative?(*args, **_arg1, &block); end

# @method_missing: delegated to BigDecimal
sig { params(args: T.untyped, _arg1: T.untyped, block: T.nilable(T.proc.void)).returns(Integer) }
def to_i(*args, **_arg1, &block); end

# @method_missing: delegated to BigDecimal
sig { params(args: T.untyped, _arg1: T.untyped, block: T.nilable(T.proc.void)).returns(Float) }
def to_f(*args, **_arg1, &block); end

# @method_missing: delegated to BigDecimal
sig { params(args: T.untyped, _arg1: T.untyped, block: T.nilable(T.proc.void)).returns(Integer) }
def hash(*args, **_arg1, &block); end
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The (*args, **_arg1, &block) arguments are necessary to appease Sorbet, because that's the signature it generates for methods delegated via def_delegators.

rbi/annotations/shopify-money.rbi Outdated Show resolved Hide resolved
value: T.nilable(T.any(Money, Numeric, String)),
currency: T.nilable(T.any(Money::Currency, Money::NullCurrency, String)),
)
.returns(Money)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is now causing errors when Money instances are multiplied with integers, 5 * Money.new as the second argument isn't an Integer https://github.com/sorbet/sorbet/blob/dd4602e030832c930d0b413f31734a83708448f6/rbi/core/integer.rbi#L59.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, yeah, that's a bummer. I can't think of an easy fix though -- we'd need to be able to overload the signatures for Integer#*, Float#*, etc. with an additional signature, and I think Sorbet only supports overloading for its own stdlib types.

Maybe this is an acceptable limitation? It's easily fixed by swapping the arguments so that Money#* is called instead. It would be nice if this could be made more obvious but I'm not sure how.

Open to any and all suggestions for improving this :)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah we can't represent the current implementation without overloading that signature.

I'm okay with swapping the arguments solution. Only problem is it's hard to communicate this moving forward.

@KaanOzkan KaanOzkan merged commit 0eae2eb into Shopify:main Apr 6, 2023
@olivier-thatch olivier-thatch deleted the olivier-shopify-money branch April 6, 2023 20:09
@Morriar Morriar added the rbi Change related to RBI annotations label Mar 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
rbi Change related to RBI annotations
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants