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.

Sign up for GitHub

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

Jump to bottom

feat(lsp): Implement textDocument/rename #8910

Merged
merged 1 commit into from
Dec 30, 2020
Merged

feat(lsp): Implement textDocument/rename #8910

merged 1 commit into from
Dec 30, 2020

Conversation

hrsh7th
Copy link
Contributor

@hrsh7th hrsh7th commented Dec 29, 2020

Related #8643

Currently, the ConfigureRequest (in cli/tsc/compiler.d.ts) does not support tsconfig.json's include/exclude field.
So the tsserver will rename symbols in only the opened text document or explicitly dependent text document.

@CLAassistant
Copy link

CLAassistant commented Dec 29, 2020 •
edited
Loading

CLA assistant check
All committers have signed the CLA.

lucacasonato
lucacasonato reviewed Dec 29, 2020
View reviewed changes
Copy link
Member

@lucacasonato lucacasonato left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks very good. Just few comments.

cli/lsp/language_server.rs Outdated Show resolved Hide resolved
cli/lsp/language_server.rs
@@ -1143,4 +1185,57 @@ mod tests {
]);
harness.run().await;
}
#[tokio::test]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great test 👍

cli/lsp/tsc.rs Outdated Show resolved Hide resolved
hrsh7th
hrsh7th commented Dec 29, 2020
View reviewed changes
cli/lsp/tsc.rs Outdated
impl RenameLocations {
pub async fn into_workspace_edit(
self,
ls: &LanguageServer,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The RenameLocations can have the locations that is not same to current document so we should accept LanguageServer to resolve line_index per each locations.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you should be able to use the same approach as DefinitionInfoAndBoundSpan::to_definition. The index_provider argument there is probably what you want.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you! I'll fix it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I fixed it with referencing DefinitionInfoAndBoundSpan::to_definition.

@lucacasonato lucacasonato mentioned this pull request Dec 29, 2020
Merged
@kitsonk
Copy link
Contributor

kitsonk commented Dec 30, 2020

Currently, the ConfigureRequest (in cli/tsc/compiler.d.ts) does not support tsconfig.json's include/exclude field.
So the tsserver will rename symbols in only the opened text document or explicitly dependent text document.

It will never support it, so the resulting behaviour is the desired behaviour.

kitsonk
kitsonk approved these changes Dec 30, 2020
View reviewed changes
Copy link
Contributor

@kitsonk kitsonk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, very nice contribution... thank you!

@kitsonk kitsonk merged commit 57b0562 into denoland:master Dec 30, 2020
@kitsonk kitsonk mentioned this pull request Dec 30, 2020
Closed
43 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kitsonk kitsonk kitsonk approved these changes

@lucacasonato lucacasonato Awaiting requested review from lucacasonato

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@hrsh7th @CLAassistant @kitsonk @lucacasonato