WIP add support for renaming function and macro #720
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
Enhanced the application by integrating rename functionality, increasing its editing capabilities. This includes supporting the preparation and execution of rename operations in various modules such as the lexical AST module, protocol request and response modules, remote control API, and server provider handling. Key additions entail aliasing for easier reference, handling new rename-related requests and responses, and implementing server state updates to acknowledge rename capabilities. The changes underscore our dedication to improving the tool's utility and ensuring a more dynamic and responsive user experience in code manipulation scenarios. Filter out the `:struct` type references Add some handler tests for `rename` Add doc for `local_module_name` Move `rename` from `code_intelligence` to `code_mod` Move the prepare logic to a individual module This PR is primarily copied from #374, but compared to the previous implementation, we have made two changes: 1. We constrained the scope of renaming, meaning that renaming a module is only allowed at its definition. This simplification reduces a lot of computations. 2. Based on the first point, our preparation no longer returns just a single local module, but instead, the entire module. This means that module renaming has become more powerful. We can not only rename a single local module but also simplify a module. For example, renaming TopLevel.Parent.Child to TopLevel.Child, or renaming some middle parts, like TopLevel.Parent.Child to TopLevel.Renamed. I personally really like this change, especially the second point, which makes module renaming much more practical. Surround the whole module when renaming happens Remove logic related to the rename function. Apply some code review suggestions Fix a bug when expanding the module Fix a merge error Apply a format module usage suggestion Update apps/remote_control/lib/lexical/remote_control/code_mod/rename.ex Co-authored-by: Steve Cohen <[email protected]> Apply the `defdelegate` suggestion Fix a bug when the descendant containing the old suffix
* Rename the file while renaming the module. It first determines whether the module meets certain hard constraints, such as having no parent, then performs some conventional checks. If both are satisfied, it proceeds to return the file renaming. * Maybe insert special folder for PhoenixWeb's module * Apply some code review changes for using `Document.Changes` Use `Document.Changes` instead of `CodeMod.Rename.DocumentChanges` Make `Document.Changes.RenameFile` a struct and use `root_module?` instead of `has_silibings?` and `has_parent?` * Use `rename progress marker` instead of suspending build * Use `file_changed` instead of `file_compile_requested` for reindexing * `uri_with_operations_counts` by client name * Apply some code review suggestions for `rename/file` * Apply suggestions from code review Co-authored-by: Steve Cohen <[email protected]> * Do not need to care about the `DidClose` message * Apply some code review suggestions 1. Move the `if-else` logic to the `Commands.Rename` module 2. Add `file_opened` message type 3. modify the format error message * Shouldn't returns `RenameFile` if the file name not changed * Change `uri_with_operation_counts` to `uri_with_expected_operation` and special some message type for emacs --------- Co-authored-by: Steve Cohen <[email protected]>
scottming
force-pushed
the
rename-function
branch
2 times, most recently
from
3df5e6c
to
03c5889
Compare
scohen
reviewed
Apr 28, 2024
Comment on lines
8
to
11
| alias Lexical.Document.Range | ||
|
|
||
| alias Lexical.RemoteControl.CodeIntelligence.Entity |
There was a problem hiding this comment.
Suggested change
| alias Lexical.Document.Range | |
| alias Lexical.RemoteControl.CodeIntelligence.Entity | |
| alias Lexical.Document.Range | |
| alias Lexical.RemoteControl.CodeIntelligence.Entity |
|
|
||
| with {:ok, _} <- Document.Store.open_temporary(uri), | ||
| {:ok, _document, analysis} <- Document.Store.fetch(uri, :analysis) do | ||
| Ast.Callable.fetch_name_range(analysis, entry.range.start, local_name) |
There was a problem hiding this comment.
Suggested change
| Ast.Callable.fetch_name_range(analysis, entry.range.start, local_name) | |
| Callable.fetch_name_range(analysis, entry.range.start, local_name) |
|
|
||
| with {:ok, _} <- Document.Store.open_temporary(uri), | ||
| {:ok, _document, analysis} <- Document.Store.fetch(uri, :analysis) do | ||
| Ast.Callable.fetch_name_range(analysis, entry.range.start, local_name) |
There was a problem hiding this comment.
Would it make more sense to use Sourceror's patch functionality here, then use Ast.patch_to_edit to turn them into edits?
There's already Sourceror.Patch.rename_call
| defp name_range({callable_name, meta, _params_and_body}, position) | ||
| when is_atom(callable_name) do | ||
| callable_name = to_string(callable_name) | ||
| start_position = %{position | character: meta[:column]} |
There was a problem hiding this comment.
Suggested change
| start_position = %{position | character: meta[:column]} | |
| start_position = %Position{position | character: start_column} |
|
|
||
| defp name_range({callable_name, meta, _params_and_body}, position) | ||
| when is_atom(callable_name) do | ||
| callable_name = to_string(callable_name) |
There was a problem hiding this comment.
start_column = meta[:column]
| when is_atom(callable_name) do | ||
| callable_name = to_string(callable_name) | ||
| start_position = %{position | character: meta[:column]} | ||
| end_position = %{position | character: meta[:column] + String.length(callable_name)} |
There was a problem hiding this comment.
Suggested change
| end_position = %{position | character: meta[:column] + String.length(callable_name)} | |
| end_position = %{position | character: start_column + String.length(callable_name)} |
Comment on lines
+44
to
+45
| defp name_range({{:., meta, [_aliases, callable_name]}, _, _}, position) do | ||
| dot_length = 1 |
There was a problem hiding this comment.
Suggested change
| defp name_range({{:., meta, [_aliases, callable_name]}, _, _}, position) do | |
| dot_length = 1 | |
| @dot_length 1 | |
| defp name_range({{:., meta, [_aliases, callable_name]}, _, _}, position) do |
| @@ -0,0 +1,62 @@ | |||
| defmodule Lexical.Ast.Callable do | |||
There was a problem hiding this comment.
you said there were problems with indexing these things, why not fix them in the indexer rather than here?
Comment on lines
+687
to
+688
| This is a bug in our function indexation: | ||
| when the cursor is at the reference for a imported call, the definition function can't be found. |
There was a problem hiding this comment.
Still wip, so I use test record the issue first
scottming
force-pushed
the
rename-function
branch
2 times, most recently
from
0806c22
to
cd3d0e1
Compare
Still WIP, only finished the function part. Add a failed test
scottming
force-pushed
the
rename-function
branch
from
cd3d0e1
to
1ec5b81
Compare
scottming
force-pushed
the
rename-module-v2
branch
11 times, most recently
from
6bdac06
to
fc4eb47
Compare
scottming
force-pushed
the
rename-module-v2
branch
from
fc4eb47
to
3262567
Compare
scottming
force-pushed
the
rename-module-v2
branch
3 times, most recently
from
b5229ff
to
8b40671
Compare
scottming
force-pushed
the
rename-module-v2
branch
4 times, most recently
from
55dafa2
to
5f2a602
Compare
scottming
force-pushed
the
rename-module-v2
branch
5 times, most recently
from
11c8ac7
to
2ffe206
Compare
scottming
force-pushed
the
rename-module-v2
branch
9 times, most recently
from
96d3ffb
to
d72a46f
Compare
scottming
force-pushed
the
rename-module-v2
branch
4 times, most recently
from
e4bb866
to
2898e78
Compare
scottming
force-pushed
the
rename-module-v2
branch
from
2898e78
to
a21b40c
Compare
scottming
force-pushed
the
rename-module-v2
branch
from
7862fb5
to
528a1c0
Compare
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.
Still WIP, only finished the function part.
CleanShot.2024-04-28.at.16.40.24.mp4