Skip to content
This repository was archived by the owner on Oct 12, 2022. It is now read-only.

Implement the 'textDocument/rename' command #337

Merged
merged 2 commits into from
Nov 26, 2018

Conversation

ccojocar
Copy link
Contributor

@ccojocar ccojocar commented Nov 5, 2018

No description provided.

Copy link
Member

@keegancsmith keegancsmith left a comment

Choose a reason for hiding this comment

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

Woah this is really really cool! I like that you just use references. It would be great to add some tests. If you look into our tests we have very comprehensive tests which should be easy to use.

@ccojocar
Copy link
Contributor Author

ccojocar commented Nov 5, 2018

Thanks! I will try to add some tests in the next days. I see that is possible to spin up a tests langserver.

@ccojocar
Copy link
Contributor Author

ccojocar commented Nov 7, 2018

@keegancsmith I added some tests for rename command. Please could you have a look? Thanks

@Dbz
Copy link

Dbz commented Nov 25, 2018

Plus 1 for this. #220

@ccojocar
Copy link
Contributor Author

@keegancsmith Any chance to get this merged soon? Thanks

@keegancsmith
Copy link
Member

ccojocar commented 19 days ago

Shit sorry, did not mean to ignore this. I'll take a look now.

Copy link
Member

@keegancsmith keegancsmith left a comment

Choose a reason for hiding this comment

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

Got an inline comment but super minor so lets just ignore it :) I'm not 100% confident that the ranges returned by references will always be the correct one w.r.t. renaming. But lets ship this and see how it behaves in practice.

}

result := lsp.WorkspaceEdit{}
if result.Changes == nil {
Copy link
Member

Choose a reason for hiding this comment

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

nit: result.Changes will always be nil here. Rather just initialize it

@keegancsmith keegancsmith merged commit d6e9e93 into sourcegraph:master Nov 26, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants