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

Bump Sorbet version and introduce AbstractParam #160

Closed
wants to merge 2 commits into from

Conversation

st0012
Copy link
Member

@st0012 st0012 commented Apr 4, 2023

Currently, the Param class plays 2 roles:

  1. Being a class that represents normal params (used in projects like Tapioca)
  2. Being the abstract class for other param types

However, after sorbet/sorbet#6273 Sorbet stops prohibiting instantiating abstract classes, which means these 2 roles can't co-exist on the same calss anymore.

I think that leaves us with 2 options:

  1. Create an AbstractParam class (I'm open for a better name) as all param classes' (including Param's) superclass
  2. Or create a new class to present normal params and migrate Param usages to it

Since 2) will require downstream projects to adopt new a new param class, it's functionally a breaking change.
And although 1) will also require changes, but it'll be on signatures only, which IMO can be argued as NOT a breaking change?

WDYT?

Required changes on Tapioca

diff --git a/lib/tapioca/helpers/rbi_helper.rb b/lib/tapioca/helpers/rbi_helper.rb
index 4796de77..f0fc1d7d 100644
--- a/lib/tapioca/helpers/rbi_helper.rb
+++ b/lib/tapioca/helpers/rbi_helper.rb
@@ -73,7 +73,7 @@ module Tapioca
       create_typed_param(RBI::BlockParam.new(name), type)
     end
 
-    sig { params(param: RBI::Param, type: String).returns(RBI::TypedParam) }
+    sig { params(param: RBI::AbstractParam, type: String).returns(RBI::TypedParam) }
     def create_typed_param(param, type)
       RBI::TypedParam.new(param: param, type: sanitize_signature_types(type))
     end
diff --git a/lib/tapioca/rbi_ext/model.rb b/lib/tapioca/rbi_ext/model.rb
index a85bb1d6..6aedc3c2 100644
--- a/lib/tapioca/rbi_ext/model.rb
+++ b/lib/tapioca/rbi_ext/model.rb
@@ -124,7 +124,7 @@ module RBI
   end
 
   class TypedParam < T::Struct
-    const :param, RBI::Param
+    const :param, RBI::AbstractParam
     const :type, String
   end
 end

dependabot bot and others added 2 commits April 4, 2023 19:32
Bumps [sorbet](https://github.com/sorbet/sorbet) from 0.5.10741 to 0.5.10746.
- [Release notes](https://github.com/sorbet/sorbet/releases)
- [Commits](https://github.com/sorbet/sorbet/commits)

---
updated-dependencies:
- dependency-name: sorbet
  dependency-type: direct:development
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <[email protected]>
Sorbet starts raising an error when an abstract class is instantiated.
And since we do need to instantiate Param, we should introduce another
class to carry the abstract methods.
@st0012 st0012 requested a review from a team as a code owner April 4, 2023 18:49
Copy link
Collaborator

@Morriar Morriar left a comment

Choose a reason for hiding this comment

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

I don't think the change should be done here. Param is already abstract so the fault is on whatever is trying to instantiate it.

From what I understand the problem is mostly because of this line in Tapioca: https://github.com/Shopify/tapioca/blob/main/lib/tapioca/helpers/rbi_helper.rb#L43 ? Should it be using ReqParam instead?

@st0012
Copy link
Member Author

st0012 commented Apr 4, 2023

@Morriar Yeah I think that makes more sense. I've opened Shopify/tapioca#1467 for it.

@st0012
Copy link
Member Author

st0012 commented Apr 4, 2023

Close in favour of #161

@st0012 st0012 closed this Apr 4, 2023
@st0012 st0012 deleted the introduce-abstract-param branch April 4, 2023 20:17
@Morriar Morriar added the dependencies Pull requests that update a dependency file label Mar 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants