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

Implement renameProvider #339

Merged
merged 9 commits into from
Oct 11, 2020
Merged

Implement renameProvider #339

merged 9 commits into from
Oct 11, 2020

Conversation

renkun-ken
Copy link
Member

@renkun-ken renkun-ken commented Sep 19, 2020

Close #337

This PR implements prepareRename and renameProvider based on the referenceProvider implemented by #338 by transforming the references into a WorkspaceEdit.

Kapture 2020-09-19 at 21 55 06

@renkun-ken renkun-ken requested a review from randy3k September 19, 2020 14:32
@randy3k
Copy link
Member

randy3k commented Sep 25, 2020

Sorry for the slow review. Will try to find some time to do it tomorrow.

@renkun-ken
Copy link
Member Author

Feel free to take your time!

We might review and merge in the following order due to dependencies:

#338, #339

@renkun-ken renkun-ken requested a review from randy3k October 5, 2020 13:11
@renkun-ken
Copy link
Member Author

@randy3k Is there anything you think should be improved?

@randy3k
Copy link
Member

randy3k commented Oct 11, 2020

LGTM. Sorry for missing it.

@renkun-ken renkun-ken merged commit 37269aa into master Oct 11, 2020
@randy3k
Copy link
Member

randy3k commented Oct 11, 2020

By the way, how's its performance for a large project?

@renkun-ken
Copy link
Member Author

renkun-ken commented Oct 11, 2020

renameProvider relies on referencesProivder which again relies on definitionProvider. The performance of renameProvider is almost equal to that of the referencesProvider.

I tried using the Find All References in projects such as languageserver, lintr, data.table, ggplot2, and knitr, and the performance looks good in most cases. In fact, its performance could be poor if we try to find a token which occurs too many times (e.g. more than 1000+) in the projects.

@randy3k
Copy link
Member

randy3k commented Oct 11, 2020

In fact, its performance could be poor if we try to find a token which occurs too many times (e.g. more than 1000+) in the projects.

That's what I was referring to. But anyway, such of use case should not be common.

@grepinsight
Copy link

👍 👍 Thank you for this feature

@ManuelHentschel
Copy link
Member

Great feature, thanks a lot! 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement renameProvider
4 participants