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 more signatures for #present? for Rails 7.2 #265

Merged
merged 1 commit into from
Jul 18, 2024

Conversation

olivier-thatch
Copy link
Contributor

@olivier-thatch olivier-thatch commented Jul 17, 2024

Type of Change

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

Changes

Rails 7.2 defines #present? methods directly on a few classes like Array, Hash, Numeric etc.

This requires new signatures, otherwise Sorbet defaults to T.untyped: https://sorbet.run/#%23%20typed%3A%20true%0A%0Aclass%20Object%0A%20%20extend%20T%3A%3ASig%0A%0A%20%20sig%20%7B%20returns%28T%3A%3ABoolean%29%20%7D%0A%20%20def%20blank%3F%0A%20%20%20%20respond_to%3F%28%3Aempty%3F%29%20%3F%20!!%28T.unsafe%28self%29.empty%3F%29%20%3A%20false%0A%20%20end%0A%0A%20%20sig%20%7B%20returns%28T%3A%3ABoolean%29%20%7D%0A%20%20def%20present%3F%0A%20%20%20%20!blank%3F%0A%20%20end%0Aend%0A%0Aclass%20Array%0A%20%20alias_method%20%3Ablank%3F%2C%20%3Aempty%3F%0Aend%0A%0Aclass%20ArrayRails72%20%3C%20Array%0A%20%20%23%20sig%20%7B%20returns%28T%3A%3ABoolean%29%20%7D%0A%20%20def%20present%3F%0A%20%20%20%20!empty%3F%0A%20%20end%0Aend%0A%0Aarr%20%3D%20%5B%5D%0AT.reveal_type%28arr.present%3F%29%0A%0Aarr_72%20%3D%20ArrayRails72.new%0AT.reveal_type%28arr_72.present%3F%29%0A

This should be safe to merge and use with previous Rails versions though, since the #present? method does exist on Array and Hash even with Rails 7.1, it's just inherited from Object rather than defined directly on the classes.

@olivier-thatch olivier-thatch requested a review from a team as a code owner July 17, 2024 22:52
@paracycle
Copy link
Member

Ah, I guess the reason why we really need this is because Tapioca will now generate versions of Array#present? and friends without signatures, which will prevent Sorbet from using the signature from the Object#present? version.

The motivation for adding these wasn't very clear to me until I understood this, so wanted to share for other onlookers.

@paracycle
Copy link
Member

paracycle commented Jul 17, 2024

I think we should also add:

class NilClass
  sig { returns(TrueClass) }
  def blank?; end

  sig { returns(FalseClass) }
  def present?; end
end

class FalseClass
  sig { returns(TrueClass) }
  def blank?; end

  sig { returns(FalseClass) }
  def present?; end
end

class TrueClass
  sig { returns(FalseClass) }
  def blank?; end

  sig { returns(TrueClass) }
  def present?; end
end

class Numeric
  sig { returns(FalseClass) }
  def blank?; end

  sig { returns(TrueClass) }
  def present?; end
end

class Time
  sig { returns(FalseClass) }
  def blank?; end

  sig { returns(TrueClass) }
  def present?; end
end

which will allow better typing for those cases as well: https://sorbet.run/#%23%20typed%3A%20true%0A%0Aclass%20Object%0A%20%20extend%20T%3A%3ASig%0A%0A%20%20sig%20%7B%20returns%28T%3A%3ABoolean%29%20%7D%0A%20%20def%20blank%3F%20%3D%20T.unsafe%28nil%29%0A%0A%20%20sig%20%7B%20returns%28T%3A%3ABoolean%29%20%7D%0A%20%20def%20present%3F%20%3D%20T.unsafe%28nil%29%0Aend%0A%0Aclass%20NilClass%0A%20%20sig%20%7B%20returns%28TrueClass%29%20%7D%0A%20%20def%20blank%3F%20%3D%20T.unsafe%28nil%29%0A%0A%20%20sig%20%7B%20returns%28FalseClass%29%20%7D%0A%20%20def%20present%3F%20%3D%20T.unsafe%28nil%29%0Aend%0A%0Aclass%20FalseClass%0A%20%20sig%20%7B%20returns%28TrueClass%29%20%7D%0A%20%20def%20blank%3F%20%3D%20T.unsafe%28nil%29%0A%0A%20%20sig%20%7B%20returns%28FalseClass%29%20%7D%0A%20%20def%20present%3F%20%3D%20T.unsafe%28nil%29%0Aend%0A%0Aclass%20TrueClass%0A%20%20sig%20%7B%20returns%28FalseClass%29%20%7D%0A%20%20def%20blank%3F%20%3D%20T.unsafe%28nil%29%0A%0A%20%20sig%20%7B%20returns%28TrueClass%29%20%7D%0A%20%20def%20present%3F%20%3D%20T.unsafe%28nil%29%0Aend%0A%0Aclass%20Numeric%0A%20%20sig%20%7B%20returns%28FalseClass%29%20%7D%0A%20%20def%20blank%3F%20%3D%20T.unsafe%28nil%29%0A%0A%20%20sig%20%7B%20returns%28TrueClass%29%20%7D%0A%20%20def%20present%3F%20%3D%20T.unsafe%28nil%29%0Aend%0A%0Aclass%20Time%0A%20%20sig%20%7B%20returns%28FalseClass%29%20%7D%0A%20%20def%20blank%3F%20%3D%20T.unsafe%28nil%29%0A%0A%20%20sig%20%7B%20returns%28TrueClass%29%20%7D%0A%20%20def%20present%3F%20%3D%20T.unsafe%28nil%29%0Aend%0A%0Aa%20%3D%2042%20unless%20nil.blank%3F%0Aif%20false.present%3F%0A%20%20puts%20%22Here%22%0Aend

@olivier-thatch
Copy link
Contributor Author

@paracycle Those all already exist :) Though some are marked as @shim, which will no longer be true for Rails 7.2 as they will become actual methods, not shims.

Copy link
Contributor

@KaanOzkan KaanOzkan left a comment

Choose a reason for hiding this comment

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

Thanks. Looks like it needs a rebase

@KaanOzkan KaanOzkan merged commit 4b124da into Shopify:main Jul 18, 2024
4 checks passed
@@ -75,11 +75,23 @@ class Object
end

class Hash
sig { returns(T::Boolean)}
Copy link
Contributor

Choose a reason for hiding this comment

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

Would you mind submitting a quick patch to fix the spacing? (We should add RuboCop to this repo hah)

Suggested change
sig { returns(T::Boolean)}
sig { returns(T::Boolean) }

Copy link
Contributor

Choose a reason for hiding this comment

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

We do run rubocop, I'll add a test for this and see what's happening.

Copy link
Contributor

Choose a reason for hiding this comment

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

@olivier-thatch olivier-thatch deleted the olivier/rails-7-2-present branch July 18, 2024 20:12
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.

4 participants