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

Symbol Rename #131

Open
wants to merge 8 commits into
base: master
Choose a base branch
from
Open

Symbol Rename #131

wants to merge 8 commits into from

Conversation

Mandarancio
Copy link

Implementation of symbol renaming using the results from the lsp server:

  • using get_token_range when no selection is active to get the symbol under the cursor
  • using the result from lsp to rename the symbol across the project:
    • open all affected document
    • select and replace all symbol occurencies

I did not implement an automatic file save of the changes as it force the developer to open, (review), and save the changes manually (it could be a configuration).

There is still work to do as the LSP after the refactoring seems to be out of sync with the updated files, help with testing and debugging is needed

@Mandarancio Mandarancio marked this pull request as draft July 10, 2024 16:23
@jgmdev
Copy link
Member

jgmdev commented Jul 10, 2024

I have implemented this some time ago with support for previewing and approving the changes graphically pragtical/lsp#3

@Mandarancio
Copy link
Author

@jgmdev nice, there are any plans to merge it into lite-xl-lsp?

@jgmdev
Copy link
Member

jgmdev commented Jul 10, 2024

Can be picked by any one for merging.

Copy link
Member

@Guldoman Guldoman left a comment

Choose a reason for hiding this comment

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

For now this approach is probably fine, and allows to relatively easily revert the changes.

In the future, if we want to add some interface for it, we'll design something (for example something like what I do in the copilot plugin for the "panel completions", but a bit more complete).

init.lua Outdated Show resolved Hide resolved
init.lua Outdated Show resolved Hide resolved
init.lua Outdated Show resolved Hide resolved
@Guldoman Guldoman linked an issue Jul 12, 2024 that may be closed by this pull request
@Mandarancio
Copy link
Author

Mandarancio commented Jul 12, 2024

I just realized that different LSP have different responses, for example clangd will reply with something like this

{
  "results": {
    "changes": {
      "$FILE_URI_0": [ ... ],
      "$FILE_URI_1": [ ... ]
    }
  }
}

While pyright will answer like:

{
  "results": {
    "documentChanges": [
      {
        "textDocument": { "uri": "$FILE_URI_0" },
        "edits": [ ... ]
      }, {
        "textDocument": { "uri": "$FILE_URI_1" },
        "edits": [ ... ]
      }
    ]
  }
}

There is a way to uniform the responses?

@Mandarancio
Copy link
Author

I just realized that different LSP have different responses, for example clangd will reply with something like this

{
  "results": {
    "changes": {
      "$FILE_URI_0": [ ... ],
      "$FILE_URI_1": [ ... ]
    }
  }
}

While pyright will answer like:

{
  "results": {
    "documentChanges": [
      {
        "textDocument": { "uri": "$FILE_URI_0" },
        "edits": [ ... ]
      }, {
        "textDocument": { "uri": "$FILE_URI_1" },
        "edits": [ ... ]
      }
    ]
  }
}

There is a way to uniform the responses?

Implemented workaround that convert the two types of answer to a same structure

Copy link
Member

@Guldoman Guldoman left a comment

Choose a reason for hiding this comment

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

A few more things:

  1. Set our client capabilities (in server.lua) to support textDocument.rename (but not the "prepared" part).
  2. Check the server capability (renameProvider) before sending the request.
  3. If the server capability is set, add a rename entry in the context menu.

for file_uri, changes in pairs(response.result.changes) do
core.log(file_uri .. " " .. #changes)
-- TODO: Finish implement textDocument/rename
if response.result then
Copy link
Member

Choose a reason for hiding this comment

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

Also check for response.error and log it out.

Copy link
Author

@Mandarancio Mandarancio Jul 18, 2024

Choose a reason for hiding this comment

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

  1. Done
  2. It was already checked
  3. I have doubt how I can access to the document servers at the time of the context menu initialisation
  4. Done

init.lua Outdated Show resolved Hide resolved
init.lua Outdated Show resolved Hide resolved
@Mandarancio Mandarancio marked this pull request as ready for review July 22, 2024 11:14
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.

Rename Symbol not working
3 participants