Rename function/variable #791
Conversation
|
@lukaszsamson thanks for the update. Sounds like it's best for me to stop looking at improvements to support rename functionality in elixir sense and elixir-ls until #773 gets merged and other existing features are ported over? Sounds like the existing elixir-sense find references functionality will also be replaced so I should close this PR too? elixir-lsp/elixir_sense#173 In the meantime let me know if there's any support that could be helpful to improve elixir-ls. I'm up for contributing but want to make sure what I do is useful! |
|
@timgent #773 has merged. There's an example of using AST in https://github.com/elixir-lsp/elixir-ls/blob/master/apps/language_server/lib/language_server/experimental/code_mod/replace_with_underscore.ex |
LSPs can offer a rename capability to rename a "symbol" across a workspace. We're not there yet, but this is a starting point. In VS Code it can be used by right clicking a symbol (currently variable or call to local function) and selecting "Rename symbol". A dialog should pop up asking for a new name. Official spec: https://microsoft.github.io/language-server-protocol/specifications/specification-current/#textDocument_rename
- Make rename_example a `.ex` file so we can later test multi-file refactors - Simplify rename_example to make it easier to follow - Extract some common patterns out in the tests
timgent
force-pushed
the
rename_symbol
branch
from
532bf88
to
f18d548
Compare
|
@timgent don't wont to push :), but is any plan to finish renaming, or you are blocking by something? |
|
@illia-danko I've since got a new baby so been struggling to find time. Any more guidance on using the AST as suggested would be helpful though. I've not had the chance to look into that in depth, but if I could get a few pointers that might make it easier :) |
|
@timgent congratulations on the baby born! Sure, take your time, hopefully you'll come back at some point |
|
AST based transforms landed in #1057. Would you be interested in reworking the PR using this approach? Possibly also with |
|
@lukaszsamson I'm still a bit time-poor to be honest but would love to get this over the line. Will try to get my head back into this and give it a go if I find the time |
|
How's the work going? I'm willing to take this feature upon myself if you don't have time now. |
|
@hubcio2115 yeah tbh I've struggled to find the time - if you're able to pick it up that would be great. I've been working mostly in Typescript recently which doesn't help! Thank you! |
|
I'm pretty new to this so it might take a while. |
|
After trying a couple of times to implement this feature I decided to give up for now. I'm not nearly enough proficient with the language to do this on my own. I'll probably go back to it after some time, if it won't be already implemented. |
Note that this uses ElixirSense to get the definition and references under the hood, which limits the rename functionality. There is a PR raised against elixir_sense already to address some of the current issues with the find references functionality - elixir-lsp/elixir_sense#173 - I'm waiting for feedback on that one.
I do suspect that there are other cases where find definition/references doesn't work 100% - for example I know find references doesn't work for a variable in a list comprehension. I've not had time to properly test and identify all of those cases, but know they will impact rename functionality. Still feel like this is a good improvement to merge in, but let me know what you think!