Fix indentation with OnTypeFormatting method for \n
#329
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
DavisVaughan
approved these changes
May 2, 2024
There was a problem hiding this comment.
I got tripped up for awhile because it currently isn't working for untitled schemed files (see the positron side PR), but once I got that figured out it seems to be working quite nicely!
I don't really mind the jitter TBH.
It has proven quite hard to break so far! (with that one exception below, which is a weird one)
lionel-
force-pushed
the
feature/indent-correct
branch
from
bf4bf21
to
2be0a28
Compare
Extract `ancestors()` iterator Extract indentation config into own struct Use `usize` everywhere to avoid conversions Propagate indentation config Simplify indent edit Respect indentation config in text edit Move `indent()` method out of `Backend` Add utils for testing text edits Don't depend on document's Dashmap ref Add indentation tests Take outermost node on the line before analysis Fix anchor matcher Check for OOB line Fix selection of node at point Update indentation config from formatting options Log format-on-type method Fix indentation of trailing whitespace Decline to indent if version does not match Fix indentation of top-level expressions Ignore formatting options if tab size differs Apply suggestions from code review Co-authored-by: Davis Vaughan <davis@rstudio.com> Remove custom formatting request
lionel-
force-pushed
the
feature/indent-correct
branch
from
2be0a28
to
abbf1f6
Compare
lionel-
force-pushed
the
feature/indent-correct
branch
from
c250505
to
9ba1c7a
Compare
| @@ -87,6 +87,7 @@ pub(crate) enum LspRequest { | |||
| References(ReferenceParams), | |||
| StatementRange(StatementRangeParams), | |||
| HelpTopic(HelpTopicParams), | |||
| OnTypeFormatting(DocumentOnTypeFormattingParams), | |||
There was a problem hiding this comment.
The mildest of preferences for OnTypeFormatting -> DocumentOnTypeFormatting to match the Params name and what we do in DocumentSymbol. Mostly just a consistency thing
Comment on lines
187
to
+191
| let doc = state.get_document_mut(uri)?; | ||
| let mut parser = parsers.get_mut(uri)?; | ||
|
|
||
| let mut parser = lsp_state | ||
| .parsers | ||
| .get_mut(uri) |
There was a problem hiding this comment.
I guess this is where we could add a get_parser_mut(uri) method on LspState to be parallel to get_document_mut() on WorldState, if we wanted to
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Adds an LSP method for
OnTypeFormattingto correct the indentation of the frontend.Addresses posit-dev/positron#1880
Addresses posit-dev/positron#2764
Addresses posit-dev/positron#2707
We currently don't enable
OnTypeFormattingper se but instead require a custom request that contains a versioned document. That is to prevent any data corruption that might result from the very unfortunate fact that our LSP server does not call handlers in message order (see posit-dev/positron#2692 (comment)). The document version allows us to detect that we are working with an outdated version and decline to format in this case. This is not ideal but will prevent corruption of user files.In the future, indentation will be provided as part of our formatter for R code. This will result in indentation that is fully consistent with our future formatter. For now the indentation approach taken here is similar to the tree-sitter indentation engine of Emacs. We provide the tree-sitter parent node of the beginning of line to an algorithm made of successive rules. These rules return an anchor node from which we indent, as well as the indentation width to add to the beginning of this node. For instance, for the indentation of top-level expressions, the anchor is the "program" node and the indentation width to add is 0. For a continuation in a chain of operators, the anchor is the topmost binary operation and the indentation is
tab_size.The formatting options come from two sources:
The user/workspace settings in Positron. To support this we now have the infrastructure for watching over Code/Positron settings. These indentation settings are rich and allow for instance the tab size to differ from the indentation size, as you would find in the R core sources.
The formatting options from the format request. These settings are more accurate because they reflect the state of the editor, which might have different settings than the current user/workspace settings if the user has changed them via the status bar in Positron. However they don't support different tab and indent sizes. If the settings are configured with differnt sizes, we just ignore the formatting options. This way we still provide decent support for this peculiar setting.
The frontend side has comprehensive integration tests but I added a bunch of unit tests in Ark too. These are supported be a new util
apply_text_edits()that should be useful elsewhere too.