-
Notifications
You must be signed in to change notification settings - Fork 60
Add prepare rename provider #268
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
Merged
cristianoc
merged 5 commits into
rescript-lang:master
from
amiralies:add-prepare-rename-provider
Jun 7, 2021
Merged
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit
Hold shift + click to select a range
5d5c9db
Add prepare rename provider
amiralies 30da51e
return false explicitly
amiralies d163d29
refactor completion
cristianoc f412eb7
Revert "refactor completion"
cristianoc 08682f5
Don't return a boolean directly, but use result instead.
cristianoc File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or 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
This file contains hidden or 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
This file contains hidden or 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
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wondering about the logic (and duplication of logic) here.
It seems intuitively, this should fire exactly when rename returns a non-empty set of changes.
Looking at the commands in
Commands.ml
, rename returns results always whenlocations
is a non-empty array. So it seems this logic here is unnecessary. Or if necessary, how comes the same logic is not in the rename command?Should this just call rename instead and check if the result is empty? Not sure why the protocol splits the logic in this way.
Also, just noticed
utils.getReferencesForPosition
was created when there was more than one use. Before this PR, there was a single use, so it should probably have been removed and inlined.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the split is because of checking rename availability without actually analyzing refs. (I.e check the cursor position is an ident)
Ill look at it tomorrow to see if we can do it better.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One difference w.r.t. rename is that the new name is not supplied. So technically, the rename command cannot be called.
I think this prepare operation makes most sense in an architecture when one has direct access to parsing the document, in which case
prepareRename
in some cases could be resolved by just parsing.In our case, we make use of the function that finds references. Note this is more expensive, but it should not matter much as rename is interactive.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh I see the command should find a range for the symbol. OK then the logic is necessary.
The only remaining comment I have is about returning a boolean directly, which seems to work on vscode but can't see supported in the spec.
Will make a suggestion for a change.