Skip to content

Conversation

@vinistock
Copy link
Member

Motivation

Closes #500

This bug was bothering me too much. Instead of trying to use the document scanner, I switched the implementation of the on type formatting end completions to just deal with document lines. This simplifies the implementation and makes it easier to address the bug, so I think it's a win-win.

Implementation

The way the lines work are as follows

if something
#  ^ break here

# previous_line = "if "
# current_line = "  something"
# next_line = nil

So, the final logic is:

  • If the current line is empty, then we broke the line at the end and so we can just put the end token normally
  • If the current line is not empty and the next line is not empty either, then it's a bit hard to know what the intent of the user was. We just return early (this is the bug fix)
  • Otherwise, we add the end token in a way that allows something to be in between

Automated Tests

Added a scenario and fixed another one that was weird.

Manual Tests

Basically fire up the LSP on this branch and play around breaking if statements. See if the end token ends up weird in any combination.

@vinistock vinistock added the bugfix This PR will fix an existing bug label Jun 22, 2023
@vinistock vinistock added this to the 2023-Q2 milestone Jun 22, 2023
@vinistock vinistock requested a review from a team as a code owner June 22, 2023 21:38
@vinistock vinistock self-assigned this Jun 22, 2023
@vinistock vinistock requested review from KaanOzkan and egiurleo June 22, 2023 21:38
@github-actions
Copy link
Contributor

Benchmark results in seconds (slowest at top)

          textDocument/completion average: 0.31877 std_dev: 0.018465
          textDocument/diagnostic average: 0.058992 std_dev: 0.01202
      textDocument/selectionRange average: 0.004794 std_dev: 0.00098
        textDocument/documentLink average: 0.002409 std_dev: 0.000431
            textDocument/codeLens average: 0.002382 std_dev: 0.000426
   textDocument/documentHighlight average: 0.002371 std_dev: 0.000381
      textDocument/documentSymbol average: 0.00237 std_dev: 0.000484
 textDocument/semanticTokens/full average: 0.002278 std_dev: 0.00054
        textDocument/foldingRange average: 0.002164 std_dev: 0.000398
textDocument/semanticTokens/range average: 0.001114 std_dev: 0.000247
           textDocument/inlayHint average: 0.000835 std_dev: 0.000308
               codeAction/resolve average: 0.000742 std_dev: 0.000191
               textDocument/hover average: 0.000724 std_dev: 0.000181
          textDocument/formatting average: 9.7e-05 std_dev: 0.000268
    textDocument/onTypeFormatting average: 9.5e-05 std_dev: 7.1e-05
          textDocument/codeAction average: 5.4e-05 std_dev: 4.4e-05


================================================================================
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

Copy link
Contributor

@egiurleo egiurleo left a comment

Choose a reason for hiding this comment

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

Nice! This implementation is much simpler!

Copy link
Contributor

@bitwise-aiden bitwise-aiden left a comment

Choose a reason for hiding this comment

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

Nice! Excited to try this out :D

@bitwise-aiden
Copy link
Contributor

Actually, do we have a way to check if an end already exists for this code block? One thing I've noticed is additional ends in cases where I edit the opening line and hit enter.

@vinistock vinistock merged commit 600b243 into main Jun 23, 2023
@vinistock vinistock deleted the vs/improve_on_type_formatting_robustness branch June 23, 2023 15:21
@shopify-shipit shopify-shipit bot temporarily deployed to production June 23, 2023 15:44 Inactive
vinistock added a commit that referenced this pull request Feb 28, 2024
…e bundle (#779)

* Make tests pass locally

* Fix debugger launch configurations when `debug` is not included in the bundle
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.

Better handle on type formatting when there's content inside a block

3 participants