-
Notifications
You must be signed in to change notification settings - Fork 5
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
Conversation
Co-authored-by: Alexander Momchilov <[email protected]>
There was a problem hiding this 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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Accidental commit?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this 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.
@@ -1,5 +1,60 @@ | |||
# typed: false | |||
|
|||
begin |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added 👍
1fd7528
to
ec9f95e
Compare
Motivation
Closes #160
It handles
else
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.