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.

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

Migrate path completion to YARP #873

Merged
merged 1 commit into from
Aug 21, 2023
Merged

Migrate path completion to YARP #873

merged 1 commit into from
Aug 21, 2023

Conversation

andyw8
Copy link
Contributor

@andyw8 andyw8 commented Aug 9, 2023 •
edited
Loading

Partially addresses #449

Co-authored with @bitwise-aiden, cherry-picked from @vinistock's WIP version, then updated.

bin/test test/requests/path_completion_test.rb is passing

@andyw8 andyw8 mentioned this pull request Aug 9, 2023
Closed
29 tasks
@github-actions
Copy link
Contributor

github-actions bot commented Aug 9, 2023

@andyw8 andyw8 force-pushed the andyw8/migrate-path-completion-to-yarp branch 2 times, most recently from 697098f to a49bdc1 Compare August 14, 2023 20:52
@github-actions
Copy link
Contributor

1 similar comment
@github-actions
Copy link
Contributor

andyw8
andyw8 commented Aug 15, 2023
View reviewed changes
test/requests/path_completion_test.rb Outdated
@@ -182,7 +182,7 @@ def test_completion_is_not_triggered_if_argument_is_not_a_string

end_position = {
line: 0,
character: document.source.rindex('"'),
character: document.source.rindex('o')
Copy link
Contributor Author

Choose a reason for hiding this comment

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

As mentioned in #881

@github-actions
Copy link
Contributor 

Benchmark results in seconds (slowest at top)

          textDocument/completion average: 0.308802 std_dev: 0.004856
          textDocument/diagnostic average: 0.051102 std_dev: 0.012211
          textDocument/definition average: 0.005978 std_dev: 0.003473
   textDocument/documentHighlight average: 0.00177 std_dev: 0.000205
               codeAction/resolve average: 0.001524 std_dev: 0.000165
               textDocument/hover average: 0.001453 std_dev: 0.000162
           textDocument/inlayHint average: 0.001316 std_dev: 0.000191
 textDocument/semanticTokens/full average: 0.001076 std_dev: 0.000277
        textDocument/documentLink average: 0.001061 std_dev: 0.000142
      textDocument/documentSymbol average: 0.00105 std_dev: 0.000137
            textDocument/codeLens average: 0.001046 std_dev: 0.00011
textDocument/semanticTokens/range average: 0.001001 std_dev: 9.3e-05
          textDocument/formatting average: 0.000983 std_dev: 0.000371
    textDocument/onTypeFormatting average: 0.000982 std_dev: 0.000135
        textDocument/foldingRange average: 0.000975 std_dev: 0.000129
      textDocument/selectionRange average: 0.000965 std_dev: 0.000158
          textDocument/codeAction average: 0.000956 std_dev: 0.000111


================================================================================
Comparison with main branch:

 textDocument/semanticTokens/full faster by 63.428 %
textDocument/semanticTokens/range faster by 48.215 %
      textDocument/documentSymbol faster by 64.283 %
        textDocument/foldingRange faster by 66.849 %
          textDocument/formatting unchanged
          textDocument/diagnostic unchanged
        textDocument/documentLink faster by 63.754 %
           textDocument/inlayHint faster by 18.744 %
      textDocument/selectionRange faster by 80.676 %
   textDocument/documentHighlight faster by 42.453 %
               textDocument/hover faster by 11.897 %
          textDocument/codeAction unchanged
    textDocument/onTypeFormatting unchanged
               codeAction/resolve faster by 10.891 %
          textDocument/completion unchanged
            textDocument/codeLens faster by 65.222 %
          textDocument/definition unchanged


================================================================================
Missing benchmarks:

RubyLsp::Requests::ShowSyntaxTree

@andyw8 andyw8 force-pushed the andyw8/migrate-path-completion-to-yarp branch 2 times, most recently from 31aaeb3 to d5460d2 Compare August 18, 2023 20:33
@andyw8 andyw8 marked this pull request as ready for review August 18, 2023 20:35
@andyw8 andyw8 requested a review from a team as a code owner August 18, 2023 20:35
@andyw8 andyw8 requested review from Morriar, st0012 and vinistock August 18, 2023 20:35
Morriar
Morriar reviewed Aug 21, 2023
View reviewed changes
lib/ruby_lsp/requests/path_completion.rb Outdated
def on_tstring_content(node)
@tree.search(node.value).sort.each do |path|
sig { params(node: YARP::StringNode).void }
def on_string_node(node)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
def on_string_node(node)
def on_string(node)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍 fixed

@andyw8 andyw8 force-pushed the andyw8/migrate-path-completion-to-yarp branch from d5460d2 to 2fa7421 Compare August 21, 2023 13:50
vinistock
vinistock approved these changes Aug 21, 2023
View reviewed changes
test/requests/path_completion_test.rb
@@ -27,7 +27,7 @@ def test_completion_command
}
end_position = {
line: 0,
character: document.source.rindex('"'),
character: T.must(document.source.rindex('"')) - 1,
Copy link
Member

Choose a reason for hiding this comment

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

These off by one errors need to be fixed in YARP. I wonder how hard it would be to help them get this done, so that we can stop altering our tests.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

#917

@andyw8 andyw8 mentioned this pull request Aug 21, 2023
Closed
@andyw8 andyw8 merged commit 61c619d into yarp Aug 21, 2023
@andyw8 andyw8 deleted the andyw8/migrate-path-completion-to-yarp branch August 21, 2023 20:37
vinistock added a commit that referenced this pull request Feb 28, 2024
@vinistock
Alert shadowenv users about missing Ruby (#873)
2bf2c35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vinistock vinistock vinistock approved these changes

@Morriar Morriar Morriar approved these changes

@st0012 st0012 Awaiting requested review from st0012 st0012 was automatically assigned from Shopify/ruby-dev-exp

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@andyw8 @Morriar @vinistock