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

handle end when there's content remaining on the line correctly #2819

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

stathis-alexander
Copy link

@stathis-alexander stathis-alexander commented Nov 3, 2024

Motivation

The current behavior of the automatic end on type is unpredictable and strange. It appends the end in weird spots and leaves the cursor in a strange location.

Some examples:

If there's content on the line, the end is inserted and the remaining content is appended after the new end:
Screenshot 2024-11-03 at 08 38 12
Screenshot 2024-11-03 at 08 36 23

If there's a set of parens and you expand to a newline, it puts autocompletes the end inside the paren:
Screenshot 2024-11-03 at 08 39 25
Screenshot 2024-11-03 at 08 36 58

I encounter this most often when breaking sigs up onto multiple lines

Screenshot 2024-11-03 at 08 41 09
Screenshot 2024-11-03 at 08 41 15

Or when attempting to break up the list of parameters:
Screenshot 2024-11-03 at 08 42 38
Screenshot 2024-11-03 at 08 42 50

Or when breaking up lines with long conditions:
Screenshot 2024-11-03 at 08 45 15
Screenshot 2024-11-03 at 08 45 29

Implementation

The current logic is a little bit mystical to me. I'm not sure why the conditions are what they are, and I couldn't find a reasonable way to tweak it to be more reasonable.

My expectation as a user is that if I hit enter, anything after my cursor on the line should always be appended within the autocorrected end block, and my cursor should be left at the beginning of the new line. There's no other natural behavior here.

The complexity is that there is some odd behavior in the parser or in the other auto-correcting behavior from the LSP itself which causes the lines provided to the handle_statement_end to behave differently.

  1. If the next character is not a ), then the current line is what you would expect.
  2. If the next character is a ), then the current line terminates at the last position, and the next line is whatever content was remaining on the line before the user entered the newline character.

In either case, the user wants the same behavior - all of the remaining content on the line should occur before the autocorrected end, and the cursor should be at the beginning of the new line they just created.

Automated Tests

I couldn't quickly figure out how to get the existing unit tests to run, but I'm thinking they should all pass first try if they're well written. I'd be happy to add additional unit tests here to test more advanced cases.

Manual Tests

Started ruby-lsp in debug mode. Here are some results from the new code.

Breaking up content:
Screenshot 2024-11-03 at 08 54 52
Screenshot 2024-11-03 at 08 54 58

If there's a set of parens and you expand to a newline:
Screenshot 2024-11-03 at 08 59 45
Screenshot 2024-11-03 at 09 00 15

breaking sigs up onto multiple lines
Screenshot 2024-11-03 at 08 57 14
Screenshot 2024-11-03 at 08 57 21

Or when attempting to break up the list of parameters:
Screenshot 2024-11-03 at 08 57 39
Screenshot 2024-11-03 at 08 57 54

Or when breaking up lines with long conditions:
Screenshot 2024-11-03 at 08 58 18
Screenshot 2024-11-03 at 08 58 28

@stathis-alexander stathis-alexander marked this pull request as ready for review November 3, 2024 14:17
@stathis-alexander stathis-alexander requested a review from a team as a code owner November 3, 2024 14:17
@stathis-alexander
Copy link
Author

stathis-alexander commented Nov 3, 2024

I'm looking at #1231 from @vinistock and seeing that there are actually two cases here.

Although I do agree that the existing behavior makes more sense in his case:

foo(proc do )

with this code becomes

foo(proc do 

  end
)

I would argue the case I'm solving for is more typical:

if foo()

becomes

if foo(
  )
end

The proper fix here would actually to be to look back to the ordering of the keyword and the paren and then use that to determine if the next paren should appear before or after the end, but we're getting into some pretty complex parsing at that point which this approach (lines & positions) feels a bit too unsophisticated to handle. The right approach here may to be to actually use the AST and try to figure out what the correct auto-complete is from that representation.

Sign up for free to 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.

1 participant