Resolve initial rename value#37691
Resolve initial rename value#37691jrieken merged 6 commits intomicrosoft:masterfrom Krzysztof-Cieslak:resolveInitialRenameValue
Conversation
|
Looks pretty good after a quick read... I have to check with my schedule but we make this happen in November... |
jrieken
added
api
editor-contrib
jrieken
left a comment
jrieken
left a comment
There was a problem hiding this comment.
This is quite good already... API needs always a few extra rounds and iterations of thinking and discussion (because never ever want to break it) but I am positive about this.
There was a problem hiding this comment.
We should just go for the first rename provider so that we always use the same for resolving the location and for the actual rename.
There was a problem hiding this comment.
We can maybe move this to a 'default' implementation of resolveRenameValue
There was a problem hiding this comment.
I'm not really sure, the code below is just setting values local to the run function. It's only small extension of the logic that was already there before.
src/vs/vscode.d.ts
Outdated
There was a problem hiding this comment.
Needs maybe some better naming here but idea is good
There was a problem hiding this comment.
I am actually wondering when we would return both, so range and text (which then would be different from the text in range?). Do you have a use-case in mind?
There was a problem hiding this comment.
We removed some automagic marshalling and UriComponent must be used now
|
@Krzysztof-Cieslak Are you still with us on this one? We have scheduled this for January. |
|
@jrieken, yes, I’ve been on vacations after Xmas. Give me day or two and I’ll get back to it. |
|
@jrieken I've rebased PR to latest master and moved to using the first provider for resolving. I'm not sure if I agree/understand second suggestion. Also, happy to hear any suggestions for the naming :) |
Krzysztof-Cieslak
changed the title
| return this._withAdapter(handle, RenameAdapter, adapter => adapter.provideRenameEdits(URI.revive(resource), position, newName)); | ||
| } | ||
|
|
||
| $resolveInitialRenameValue(handle: number, resource: URI, position: IPosition): TPromise<modes.RenameInitialValue> { |
There was a problem hiding this comment.
The URI argument won't be of that type at runtime. That's because the function will be called by our JSON-rpc internals and the runtime type will be UriComponents (from which you then create an URI using revive).
src/vs/vscode.d.ts
Outdated
There was a problem hiding this comment.
I am actually wondering when we would return both, so range and text (which then would be different from the text in range?). Do you have a use-case in mind?
| */ | ||
| provideRenameEdits(document: TextDocument, position: Position, newName: string, token: CancellationToken): ProviderResult<WorkspaceEdit>; | ||
|
|
||
| resolveInitialRenameValue?(document: TextDocument, position: Position, token: CancellationToken): ProviderResult<RenameInitialValue>; |
There was a problem hiding this comment.
Maybe RenameInformation? That would make it:
resolveRenameInformation?(document: TextDocument, position: Position, token: CancellationToken): ProviderResult<RenameInformation>;
provideRenameInformation?(document: TextDocument, position: Position, token: CancellationToken): ProviderResult<RenameInformation>;There was a problem hiding this comment.
Or something more clever sounding, like RenameForesight, RenamePreflight, etc...
|
Pulling this in but marking the new API as proposed which buys as some time to polish names etc. @Krzysztof-Cieslak Thanks for good work. |
|
@jrieken, thanks for merging! |
3 participants
PR implements the functionality discussed in #7340. It's just a proposal, I've done it mostly to learn a bit about architecture around extensions API, so feel free to close it if it's not going in good direction.
CC: @aeschli & @jrieken