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

Replace Class with T::Class[T.anything] in lhm-shopify RBI #168

Closed
wants to merge 1 commit into from

Conversation

egiurleo
Copy link
Contributor

@egiurleo egiurleo commented Jun 12, 2023

Sorbet now considers Class to be a generic
(sorbet/sorbet#6781), which means that annotations continuing to use the bare Class in type signatures will cause type checking errors on Sorbet versions [0.5.10820.20230511164542-d94b3e0b2](https://github.com/sorbet/sorbet/releases/tag/0.5.10820.20230511164542-d94b3e0b2) and newer.

This PR changes two instances of bare Class to T::Class[T.anything] in the lhm-shopify RBI.

Note

I'm not sure we have a team consensus on what to do with centralized RBIs when Sorbet makes a breaking change. In my mind, the justification for making this change sooner rather than later is that by default, we'd like to support the "golden path" (updating quickly) and then we can provide extra guidance for those on older versions if necessary. I'm open to feedback though!

Once we agree on an approach, I can also find and fix instances in other gem RBIS.

Sorbet now considers `Class` to be a generic
(sorbet/sorbet#6781), which means that
annotations continuing to use the bare `Class` in type signatures will
cause type checking errors on newer versions of Sorbet.
@egiurleo egiurleo requested a review from a team as a code owner June 12, 2023 20:31
@egiurleo egiurleo requested review from Morriar and st0012 June 12, 2023 20:31
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.

Updating for latest Sorbet is good. If users can't bump Sorbet they could reject the annotations. And in the future users that need the T::Class[T.anything] syntax will only increase in comparison.

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.

I am not sure this is the right way to solve it, since Class is NOT an error at typed: true.

Moreover, I don't understand why we set any RBI file to anything above typed: true since there is not anything anyone can do with a stronger strictness level for an RBI file. That's why Tapioca generates RBI files at typed: true by default.

I think the correct and backward compatible fix here is to change the file to be typed: true.

@Morriar
Copy link
Contributor

Morriar commented Jun 13, 2023

We chose typed: strict to enforce signatures but I'm open to reevaluate.

@paracycle
Copy link
Member

We chose typed: strict to enforce signatures but I'm open to reevaluate.

Can we not get the same benefit by temporarily changing the strictness to strict on CI for an RBI file so that it fails if it doesn't have signatures, but keep it as typed: true regardless?

@egiurleo
Copy link
Contributor Author

Can we not get the same benefit by temporarily changing the strictness to strict on CI for an RBI file so that it fails if it doesn't have signatures

In that case, wouldn't it also fail if is used bare Class in its type signatures, like the current lhm-shopify RBI?

@paracycle
Copy link
Member

In that case, wouldn't it also fail if is used bare Class in its type signatures, like the current lhm-shopify RBI?

Not necessarily. If the goal with typed: strict is to only ensure that sigs are available for all the methods, that we can restrict the type check with typed: strict to just that error (7017, I believe) to enforce that. Everything else can operate as typed: true.

@paracycle
Copy link
Member

Actually, I am not sure if anyone tested this but it seems missing sigs in RBI files at typed: strict or stronger does not raise any errors anyway:

$ echo "
# typed: strong

module Lhm
  def foo
  end
end
" > lhm.rbi
$ bundle exec srb tc --no-config lhm.rbi
No errors! Great job.

but

$ echo "
# typed: strong

module Lhm
  def foo
  end
end
" > lhm.rb

$ bundle exec srb tc --no-config lhm.rb
lhm.rb:5: The method foo does not have a sig https://srb.help/7017
     5 |  def foo
          ^^^^^^^
  Autocorrect: Use `-a` to autocorrect
    lhm.rb:5: Insert sig { returns(NilClass) }
     5 |  def foo
          ^
    lhm.rb:5: Insert extend T::Sig
     5 |  def foo
        ^
Errors: 1

@Morriar
Copy link
Contributor

Morriar commented Jun 15, 2023

Good catch, I think we can relax the typed constraint and rely on https://github.com/Shopify/rubocop-sorbet/blob/main/manual/cops_sorbet.md#sorbetenforcesignatures to enforce signatures.

@paracycle
Copy link
Member

I made the change to typed: true here: #171

@paracycle paracycle closed this Aug 2, 2023
@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