Skip to content

Migrate inlay hints to yarp - #871

Merged
andyw8 merged 2 commits into
yarpfrom
andyw8/migrate-inlay-hints-to-yarp
Aug 15, 2023
Merged

Migrate inlay hints to yarp#871
andyw8 merged 2 commits into
yarpfrom
andyw8/migrate-inlay-hints-to-yarp

Conversation

@andyw8

@andyw8andyw8 commented Aug 9, 2023

Copy link
Copy Markdown
Contributor

Co-authored with @bitwise-aiden

Partially addresses #449

bin/test test/requests/inlay_hints_expectations_test.rb passes

@andyw8andyw8 mentioned this pull request Aug 9, 2023
29 tasks
@andyw8
andyw8 changed the base branch from main to yarpAugust 9, 2023 19:34
@github-actions

Copy link
Copy Markdown
Contributor

@andyw8
andyw8force-pushed the andyw8/migrate-inlay-hints-to-yarp branch 2 times, most recently from e940965 to 656c461CompareAugust 14, 2023 19:21
Comment threadlib/ruby_lsp/requests/inlay_hints.rb Outdated
exception = node.exception
return unless exception.nil? || exception.exceptions.nil?
sig { params(node: T.nilable(YARP::Node)).void }
def on_node(node)

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Since YARP doesn't support visit_rescue, I'm not sure if there's a better way to do this?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Since YARP doesn't support visit_rescue

Is there a reason for this?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

@vinistock reading https://github.com/ruby/yarp/blob/47fe0daddf2a2eab765e6b3ebff744d3fa614786/lib/yarp.rb#L152-L155, it sounds like we would need to implement a number of visit_ methods, is that the approach we want to take?

@andyw8
andyw8 marked this pull request as ready for review August 14, 2023 19:21
@andyw8
andyw8 requested a review from a team as a code ownerAugust 14, 2023 19:21
@github-actions

Copy link
Copy Markdown
Contributor

1 similar comment
@github-actions

Copy link
Copy Markdown
Contributor

@vinistockvinistock left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think it's called visit_rescue_node. Also, this had already been migrated, I have the commit 9e19894 Migrate inlay hint in my original vs/migrate_to_yarp branch.

@andyw8
andyw8force-pushed the andyw8/migrate-inlay-hints-to-yarp branch from 656c461 to 7cbff4fCompareAugust 15, 2023 15:42
@andyw8
andyw8force-pushed the andyw8/migrate-inlay-hints-to-yarp branch from 7cbff4f to daa79f9CompareAugust 15, 2023 15:43
@andyw8
andyw8force-pushed the andyw8/migrate-inlay-hints-to-yarp branch from daa79f9 to 54f2472CompareAugust 15, 2023 15:44
@andyw8

Copy link
Copy Markdown
ContributorAuthor

Updated to use Vini's version, I had missed checking what was already done.

@andyw8
andyw8 requested a review from vinistockAugust 15, 2023 15:45
@github-actions

Copy link
Copy Markdown
Contributor

2 similar comments
@github-actions

Copy link
Copy Markdown
Contributor

@github-actions

Copy link
Copy Markdown
Contributor

@andyw8
andyw8 merged commit 4f4ff35 into yarpAug 15, 2023
@andyw8
andyw8 deleted the andyw8/migrate-inlay-hints-to-yarp branch August 15, 2023 19:10
Sign up for freeto 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.

3 participants

@andyw8@Morriar@vinistock