Skip to content

Migrate document symbol to YARP - #894

Merged
andyw8 merged 1 commit into
yarpfrom
andyw8/migrate-document-symbol-to-yarp
Aug 16, 2023
Merged

Migrate document symbol to YARP#894
andyw8 merged 1 commit into
yarpfrom
andyw8/migrate-document-symbol-to-yarp

Conversation

@andyw8

@andyw8andyw8 commented Aug 16, 2023•
edited
Loading

Copy link
Copy Markdown
Contributor

Partially addresses #449

Based on @vinistock's commit in 82e975e, but I had trouble cherry-picking it so this was a partially manual effort.

bin/test test/requests/document_symbol_expectations_test.rb passes

@github-actions

github-actionsBot commented Aug 16, 2023

Copy link
Copy Markdown
Contributor

@andyw8
andyw8force-pushed the andyw8/migrate-document-symbol-to-yarp branch 2 times, most recently from fb8f25d to b0b882fCompareAugust 16, 2023 15:17
@github-actions

github-actionsBot commented Aug 16, 2023

Copy link
Copy Markdown
Contributor

1 similar comment
@github-actions

github-actionsBot commented Aug 16, 2023

Copy link
Copy Markdown
Contributor

andyw8
andyw8 commented Aug 16, 2023
View reviewed changes
Comment threadlib/ruby_lsp/requests/document_symbol.rb
:on_const_path_field,
:on_call,
:on_constant_path_write_node,
:on_constant_write_node,

@andyw8andyw8Aug 16, 2023

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 this wasn't in your original commit but I found I had to add it, maybe due to a YARP change.

@MorriarMorriarAug 16, 2023

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.

@andyw8 shouldn't we also remove the node suffix here?

@andyw8andyw8Aug 16, 2023

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.

Indeed, fixed in 768b8c6 on the yarp branch.

@andyw8
andyw8 marked this pull request as ready for review August 16, 2023 15:22
@andyw8
andyw8 requested a review from a team as a code ownerAugust 16, 2023 15:22
@andyw8andyw8 mentioned this pull request Aug 16, 2023
Closed
29 tasks
@github-actions

github-actionsBot commented Aug 16, 2023•
edited
Loading

Copy link
Copy Markdown
Contributor
Benchmark results in seconds (slowest at top)
textDocument/diagnostic average: 0.044024 std_dev: 0.011112
textDocument/definition average: 0.004819 std_dev: 0.002908
textDocument/documentHighlight average: 0.00145 std_dev: 0.000235
codeAction/resolve average: 0.001302 std_dev: 0.000331
textDocument/hover average: 0.001246 std_dev: 0.000337
textDocument/inlayHint average: 0.001117 std_dev: 0.000221
textDocument/semanticTokens/full average: 0.000944 std_dev: 0.000305
textDocument/documentLink average: 0.000923 std_dev: 0.000173
textDocument/codeLens average: 0.000914 std_dev: 0.00021
textDocument/documentSymbol average: 0.000908 std_dev: 0.000257
textDocument/codeAction average: 0.000903 std_dev: 0.000342
textDocument/foldingRange average: 0.000887 std_dev: 0.00028
textDocument/onTypeFormatting average: 0.000884 std_dev: 0.000173
textDocument/selectionRange average: 0.000877 std_dev: 0.00028
textDocument/semanticTokens/range average: 0.000854 std_dev: 0.000142
textDocument/formatting average: 0.000851 std_dev: 0.000366
textDocument/completion average: 0.000847 std_dev: 0.000197
================================================================================
Comparison with main branch:
textDocument/semanticTokens/full faster by 62.14 %
textDocument/semanticTokens/range faster by 47.143 %
textDocument/documentSymbol faster by 63.878 %
textDocument/foldingRange faster by 63.118 %
textDocument/formatting unchanged
textDocument/diagnostic unchanged
textDocument/documentLink faster by 64.804 %
textDocument/inlayHint faster by 18.849 %
textDocument/selectionRange faster by 79.889 %
textDocument/documentHighlight faster by 45.306 %
textDocument/hover unchanged
textDocument/codeAction unchanged
textDocument/onTypeFormatting unchanged
codeAction/resolve unchanged
textDocument/completion faster by 99.66 %
textDocument/codeLens faster by 66.538 %
textDocument/definition unchanged
================================================================================
Missing benchmarks:
RubyLsp::Requests::ShowSyntaxTree
RubyLsp::Requests::PathCompletion

Morriar
Morriar reviewed Aug 16, 2023
View reviewed changes
Comment threadlib/ruby_lsp/requests/document_symbol.rb Outdated
:on_top_const_field,
:on_var_field,
:on_instance_variable_write_node,
:on_class_variable_write_node,

This comment was marked as resolved.

Sign in to view

This comment was marked as resolved.

Sign in to view

@vinistockvinistockAug 16, 2023

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.

Was it updated? I'm still seeing the _node.

@andyw8andyw8Aug 16, 2023

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.

Fixed.

@andyw8
andyw8 requested a review from vinistockAugust 16, 2023 15:58
@andyw8
andyw8force-pushed the andyw8/migrate-document-symbol-to-yarp branch from b0b882f to 6c82e08CompareAugust 16, 2023 18:20
@andyw8
andyw8 merged commit b537ca5 into yarpAug 16, 2023
@andyw8
andyw8 deleted the andyw8/migrate-document-symbol-to-yarp branch August 16, 2023 18:23
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@MorriarMorriarMorriar left review comments
@vinistockvinistockvinistock approved these changes
@KaanOzkanKaanOzkanAwaiting requested review from KaanOzkanKaanOzkan was automatically assigned from Shopify/ruby-dev-exp
+1 more reviewer
@bitwise-aidenbitwise-aidenbitwise-aiden approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@andyw8@Morriar@vinistock@bitwise-aiden