Skip to content
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

How to get the resolved rename range, or should we drop range? #48700

Closed
octref opened this issue Apr 25, 2018 · 3 comments
Closed

How to get the resolved rename range, or should we drop range? #48700

octref opened this issue Apr 25, 2018 · 3 comments
Assignees
Labels
api editor-symbols definitions, declarations, references under-discussion Issue is under discussion for relevance, priority, approach verified Verification succeeded
Milestone

Comments

@octref
Copy link
Contributor

octref commented Apr 25, 2018

Testing #48409

After resolving a rename location, there is no direct way to get the returned range. It's possible for extension author to save the range somewhere and retrieve it in provideRenameEdits, but this seems cumbersome.

This API naming pattern is similar to CompletionItemProvider but work in the reverse way. Originally I thought it would resolve the range for the returned edit from provideRenameEdits.

I think an alternative might be

resolveRenamePlaceholder?(document: TextDocument, position: Position, token: CancellationToken): ProviderResult<string>;

The range is not being used by VS Code anyway. Dropping it would make it clear it's the extension author's responsibility to provide range in provideRenameEdits. He can run document.getText(range) to get the placeholder text.

@mjbvz Also what's your thoughts since you are testing this too?

@octref octref added the api label Apr 25, 2018
@octref octref added this to the April 2018 milestone Apr 25, 2018
@mjbvz
Copy link
Contributor

mjbvz commented Apr 25, 2018

Yes, I also did not understand how range was used when first trying to implement the API. Maybe the resolved rename context (range and placeholder) should be passed on provideRenameEdits, like we do for CompletionContext

@jrieken
Copy link
Member

jrieken commented Apr 26, 2018

save the range somewhere and retrieve it in provideRenameEdits, but this seems cumbersome.

Why would you want that? The language brain should be able to compute the range of symbol at a position, even two times in a row. The resolve-call is only to support the UI so that it can decide how to position the input-widget. I don't see a reason to feed that range back to the provider.

@jrieken jrieken added under-discussion Issue is under discussion for relevance, priority, approach editor-symbols definitions, declarations, references labels Apr 26, 2018
@jrieken
Copy link
Member

jrieken commented Apr 26, 2018

Talked to @kieferrm and we believe part of the confusion might be the name. We figure we can tackle this in two steps

  1. For April: rename the resolveRenameLocation-function to prepareRename
  2. For later ( and optionally) pass the result of rename to the provideRenameEdits-function.

The latter should only be needed when computing the location/range (or some other yet unknown piece of information) is very expensive and shouldn't be repeated. Then we can use the XYZContext-trick.

jrieken added a commit that referenced this issue Apr 26, 2018
@jrieken jrieken closed this as completed Apr 26, 2018
@RMacfarlane RMacfarlane added the verified Verification succeeded label Apr 27, 2018
@vscodebot vscodebot bot locked and limited conversation to collaborators Jun 10, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api editor-symbols definitions, declarations, references under-discussion Issue is under discussion for relevance, priority, approach verified Verification succeeded
Projects
None yet
Development

No branches or pull requests

4 participants