Skip to content

Allow translation of abstract methods #765

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 4 commits into from
Jun 17, 2025
Merged

Allow translation of abstract methods #765

merged 4 commits into from
Jun 17, 2025

Conversation

Morriar
Copy link
Collaborator

@Morriar Morriar commented Jun 13, 2025

Goes from this:

class Foo
  sig { abstract.void }
  def foo; end
end

to this:

class Foo
  # @abstract
  #: -> void
  def foo; end
end

Should we also insert the raise in the body while at it?

@Morriar Morriar self-assigned this Jun 13, 2025
@Morriar Morriar added the feature New feature label Jun 13, 2025
@Morriar Morriar requested a review from a team as a code owner June 13, 2025 18:33
@Morriar Morriar mentioned this pull request Jun 13, 2025
Merged
paracycle
paracycle reviewed Jun 13, 2025
View reviewed changes
Copy link
Member

@paracycle paracycle left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I think we should insert:

raise NotImplementedError, "Abstract method called"

automatically if Sorbet is then later going to require it anyway.

juharris
juharris approved these changes Jun 13, 2025
View reviewed changes
Copy link

@juharris juharris left a comment

Choose a reason for hiding this comment

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

Nice thanks! We were just noticing this on our team.

@Morriar Morriar requested a review from paracycle June 13, 2025 21:14
@Morriar
Copy link
Collaborator Author

Morriar commented Jun 13, 2025

Also added an autocorrect on the body to insert the raise 👍

KaanOzkan
KaanOzkan approved these changes Jun 16, 2025
View reviewed changes
test/spoom/sorbet/translate/sorbet_sigs_to_rbs_comments_test.rb Outdated
@@ -74,19 +74,29 @@ def a

def test_does_not_translate_to_rbs_abstract_methods
Copy link
Contributor

Choose a reason for hiding this comment

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

Test name needs updating

@Morriar Morriar force-pushed the at-rbs-abstract-translate branch from 6496b0a to 5e302a0 Compare June 16, 2025 19:31
Morriar added 4 commits June 16, 2025 15:32
@Morriar
Allow translation of abstract methods
2a2aa76 
Signed-off-by: Alexandre Terrasa <alexandre.terrasa@shopify.com>
@Morriar
Insert raise body to abstract methods
ccf3ac2 
Signed-off-by: Alexandre Terrasa <alexandre.terrasa@shopify.com>
@Morriar
Handle abstract methods with parameters
de43ceb 
Signed-off-by: Alexandre Terrasa <alexandre.terrasa@shopify.com>
@Morriar
Handle abstract setters
bf705e0 
Signed-off-by: Alexandre Terrasa <alexandre.terrasa@shopify.com>
@Morriar Morriar force-pushed the at-rbs-abstract-translate branch from 5e302a0 to bf705e0 Compare June 16, 2025 19:32
@Morriar Morriar merged commit 84e9df2 into main Jun 17, 2025
8 checks passed
@Morriar Morriar deleted the at-rbs-abstract-translate branch June 17, 2025 13:43
@Morriar Morriar mentioned this pull request Jun 17, 2025
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@juharris juharris juharris approved these changes

@KaanOzkan KaanOzkan KaanOzkan approved these changes

@jesse-shopify jesse-shopify jesse-shopify approved these changes

@paracycle paracycle Awaiting requested review from paracycle

Assignees

@Morriar Morriar

Labels
feature New feature
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@Morriar @paracycle @juharris @KaanOzkan @jesse-shopify