- Notifications
You must be signed in to change notification settings - Fork61
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Add prepare rename provider#268
Uh oh!
There was an error while loading.Please reload this page.
Conversation
this shows a feedback that the current position is not valid to be renamed to the user. letabc=123^ for some reason the |
cristianoc commentedJun 7, 2021 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
(pushed a commit by mistake on this branch). I was going to force-remove it, but don't know if you have some pending changes, so I'll let you remove it. |
server/src/server.ts Outdated
| letresult:p.Range|null=null; | ||
| if(locations!==null&&locations.length>0){ | ||
| lettargetLoc=locations.find(loc=>{ |
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 inCommands.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 noticedutils.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 caseprepareRename 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.
Feel free to apply changes to this branch |
@amiralies see my proposed change. The only real logical difference is avoiding to return a boolean directly. |
The boolean return was inside |
That only shows my little knowledge of the language. I did not know that return works like that! |
Still, even if the semantics is correct, the readability would not be great. The only way to correctly interpret such a return as a reader would be to spot the little symbol |
Looks good! |
No description provided.