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

Translate rescue node #277

Merged
merged 4 commits into from
Oct 15, 2024
Merged

Translate rescue node #277

merged 4 commits into from
Oct 15, 2024

Conversation

st0012
Copy link
Member

@st0012 st0012 commented Oct 14, 2024

Motivation

Closes #160

It handles

  • Common rescue cases
    begin # or def some_method
      foo
    rescue bar => e
      puts "rescued #{e}"
    end
  • Multiple rescues
    begin
      foo
    rescue FooError
    rescue BarError
    end
  • Rescue with else
    begin # or def some_method
      foo
    rescue bar => e
      puts "rescued #{e}"
    else
      puts "else"
    end

The first commit adds an empty Gemfile so we can use Ruby LSP in this project for its Show syntax tree command, which is important when translating nodes.

Big shoutout to @amomchilov for pairing with me on this to save me from the compiling error hell 😄

Test plan

See included automated tests.

@st0012 st0012 self-assigned this Oct 15, 2024
@st0012 st0012 requested review from amomchilov and egiurleo October 15, 2024 14:22
Copy link

@amomchilov amomchilov left a comment

Choose a reason for hiding this comment

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

Excellent work, and well tested!

Gemfile Outdated

Choose a reason for hiding this comment

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

Accidental commit?

Copy link
Member Author

Choose a reason for hiding this comment

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

No it's intentional:

The first commit adds an empty Gemfile so we can use Ruby LSP in this project for its Show syntax tree command, which is important when translating nodes.

Choose a reason for hiding this comment

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

Hmmmmm... I'm not a fan of this.

Ruby LSP shouldn't require this for a feature that's purely syntactic, and doesn't require any of the indexer information.

I'm OK to commit this for now, but we should remove it before upstreaming into sorbet/sorbet. Perhaps now is a good time to start an issue to track a to-dos like this, that we should do before merging upstream.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah that's the idea: adding it now for our workflow and remove it (along with tasks.json probably) when we upstream.

Copy link
Member Author

Choose a reason for hiding this comment

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

parser/prism/Translator.cc Outdated Show resolved Hide resolved
parser/prism/Translator.cc Outdated Show resolved Hide resolved
Copy link

@egiurleo egiurleo left a comment

Choose a reason for hiding this comment

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

This is awesome. I left some comments about small things, but they're not blockers.

parser/prism/Translator.cc Outdated Show resolved Hide resolved
@@ -1,5 +1,60 @@
# typed: false

begin

Choose a reason for hiding this comment

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

Would you mind adding comments to these examples explaining what's different about each of them so this test file is easier to glance through?

Copy link
Member Author

Choose a reason for hiding this comment

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

Added 👍

@st0012 st0012 merged commit 605b279 into prism Oct 15, 2024
1 check passed
@st0012 st0012 deleted the translate-rescue branch October 15, 2024 16:59
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.

Implement translation for PM_RESCUE_NODE
3 participants