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

refactor: make LspApplyDocumentEditCommand take plain TextEdits #2393

Merged
merged 6 commits into from
Jan 21, 2024

Conversation

rchl
Copy link
Member

@rchl rchl commented Jan 13, 2024

Refactor per discussion in #2389 (comment)

  • Make lsp_apply_document_edit take [TextEdit] rather than parsed custom structure.
  • Remove private and deprecated LSP.plugin.core.edit.apply_workspace_edit function (use session.apply_workspace_edit_async instead)
  • Remove private LSP.plugin.formatting.apply_text_edits_to_view function (use LSP.plugin.apply_text_edits instead)
  • Move LSP.plugin.core.edit.parse_text_edit to LSP.plugin.edit._parse_text_edit (and make "private")
  • Change return value of private LSP.plugin.core.edit.parse_workspace_edit function (returns unparsed edits)
  • Expose apply_text_edits as a public API

In one test I had to change the test for overflow handling from passing sys.maxsize + 1 offset to UINT_MAX + 1 offset because ST API won't allow sys.maxsize + 1. I guess this could be one minor reason why parsing before triggering lsp_apply_document_edit was there but since LSP spec limits to UINT_MAX anyway, it should be fine.

Note that changes to those private APIs would break a couple of LSP packages so I'll be creating PRs for those and linking here.

Also note that I've exposed apply_text_edits as a public API since I am very much in favor of exposing an API function rather than LSP packages calling the ST command directly since the latter is not type-checked.

Needs to be released simultaneously with:

plugin/core/edit.py Outdated Show resolved Hide resolved
@predragnikolic
Copy link
Member

When I saw the PR, I immediately wrote a list of plugins affected by this change:

Remove private LSP.plugin.formatting.apply_text_edits_to_view function (use LSP.plugin.apply_text_edits instead):

The following plugins should be updated:
~/.config/sublime-text/Packages/LSP-cspell/plugin.py:
    5: from LSP.plugin.formatting import apply_text_edits_to_view

~/.config/sublime-text/Packages/LSP-rust-analyzer/plugin_commands.py:
   10: from LSP.plugin.formatting import apply_text_edits_to_view

Move LSP.plugin.core.edit.parse_text_edit to LSP.plugin.edit._parse_text_edit (and make "private")

LSP tests and LSP-json needs to be updated:

~/.config/sublime-text/Packages/LSP-json/commands.py:
    3: from LSP.plugin.core.edit import parse_text_edit
   39:         edits = [parse_text_edit(change) for change in response]
   40          self.view.run_command('lsp_apply_document_edit', {'changes': edits}


Change return value of private LSP.plugin.core.edit.parse_workspace_edit function (returns unparsed edits)
   ~/.config/sublime-text/Packages/LSP-jdtls/modules/workspace_execute_command_handler.py:
    5: from LSP.plugin.core.edit import apply_workspace_edit, parse_workspace_edit
   45  def _apply_workspace_edit(session: Session, done: Callable[[], None], *arguments):
   46:     changes = parse_workspace_edit(arguments[0])

~/.config/sublime-text/Packages/LSP-jdtls/modules/test_extension_commands.py:
    6: from LSP.plugin.core.edit import WorkspaceEdit, parse_workspace_edit
   56:             parsed_worspace_edit = parse_workspace_edit(workspace_edit)

... I continued to read the PR description only to find out that this work had already been done :)

@rchl
Copy link
Member Author

rchl commented Jan 14, 2024

I guess it's good that you've double checked. I might be missing some repos although it seems like we are aligned in this case.

@rchl rchl merged commit 7ddde2d into main Jan 21, 2024
4 checks passed
@rchl rchl deleted the refactor/lsp-apply-document-edit branch January 21, 2024 18:59
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.

3 participants