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

Use parse! for OptionParser #958

Merged
merged 1 commit into from
Aug 31, 2023
Merged

Conversation

vinistock
Copy link
Member

@vinistock vinistock commented Aug 31, 2023

Motivation

In #905, we ended up deciding to go with parse instead of parse! because it didn't mutate ARGV. However, it also doesn't invoke the on blocks and so ruby-lsp --version or ruby-lsp --branch don't work.

Implementation

Changed the implementation to dup the ARGV first and then just used parse!.

Automated Tests

I think we need to extract a CLI class to encapsulate all of these and make it easier to test. However, I don't think it's worth the investment immediately, let's just fix this for now and think about the CLI later.

@vinistock vinistock added the bugfix This PR will fix an existing bug label Aug 31, 2023
@vinistock vinistock added this to the 2023-Q3 milestone Aug 31, 2023
@vinistock vinistock self-assigned this Aug 31, 2023
@vinistock vinistock requested a review from a team as a code owner August 31, 2023 13:21
@vinistock vinistock requested review from st0012 and KaanOzkan August 31, 2023 13:21
Copy link
Contributor

@andyw8 andyw8 left a comment

Choose a reason for hiding this comment

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

Should we report this is a bug (or documention issue) for OptionParser? It seems very non-obvious.

@vinistock
Copy link
Member Author

The usage is documented here. You basically need to pass the arguments to parse and you receive it back as the return.

It's not super intuitive when you compare the two usages, but I should have paid more attention to the docs.

@github-actions
Copy link
Contributor

Benchmark results in seconds (slowest at top)

          textDocument/completion average: 0.217097 std_dev: 0.007082
          textDocument/diagnostic average: 0.043126 std_dev: 0.011519
          textDocument/definition average: 0.005183 std_dev: 0.003017
      textDocument/selectionRange average: 0.004413 std_dev: 0.000434
   textDocument/documentHighlight average: 0.002822 std_dev: 0.000272
            textDocument/codeLens average: 0.002731 std_dev: 0.000259
        textDocument/documentLink average: 0.002647 std_dev: 0.000252
      textDocument/documentSymbol average: 0.002628 std_dev: 0.000248
 textDocument/semanticTokens/full average: 0.002613 std_dev: 0.000337
        textDocument/foldingRange average: 0.002576 std_dev: 0.0002
textDocument/semanticTokens/range average: 0.001782 std_dev: 0.000144
               codeAction/resolve average: 0.001564 std_dev: 0.000141
               textDocument/hover average: 0.001515 std_dev: 0.000112
           textDocument/inlayHint average: 0.001482 std_dev: 0.000154
    textDocument/onTypeFormatting average: 0.00094 std_dev: 9.9e-05
          textDocument/formatting average: 0.000908 std_dev: 0.000312
          textDocument/codeAction average: 0.000889 std_dev: 0.000101


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

 textDocument/semanticTokens/full unchanged
textDocument/semanticTokens/range unchanged
      textDocument/documentSymbol unchanged
        textDocument/foldingRange unchanged
          textDocument/formatting unchanged
          textDocument/diagnostic unchanged
        textDocument/documentLink unchanged
           textDocument/inlayHint unchanged
      textDocument/selectionRange unchanged
   textDocument/documentHighlight unchanged
               textDocument/hover unchanged
          textDocument/codeAction unchanged
    textDocument/onTypeFormatting unchanged
               codeAction/resolve unchanged
          textDocument/completion unchanged
            textDocument/codeLens unchanged
          textDocument/definition unchanged


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

RubyLsp::Requests::ShowSyntaxTree

@vinistock vinistock merged commit d361007 into main Aug 31, 2023
31 checks passed
@vinistock vinistock deleted the vs/use_parse_bang_for_options branch August 31, 2023 13:35
andyw8 pushed a commit that referenced this pull request Sep 5, 2023
vinistock pushed a commit that referenced this pull request Feb 28, 2024
…d-patch-0fd7d1b0d4

Bump the minor-and-patch group with 3 updates
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugfix This PR will fix an existing bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants