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

Add Local Refactoring support #70

Closed
wants to merge 3 commits into from

Conversation

rockbruno
Copy link
Contributor

This gives the LSP the ability to recognize and process sourcekitd's code refactoring procedures through code action commands.

screen shot 2019-01-16 at 5 27 23 pm

Things to take in mind:

  • This conflicts with [WIP] Add support for code action quick fixes #19 due to the codeAction addition, but it's gonna take some time for me to get this fully working so the conflict could be gone by then.
  • The current version shows everything sourcekitd supports for a specific range, but the final version will have a way to filter unsupported procedures such as Global Renaming.
  • The LSP supports several refactoring "kinds" so the client can properly separate them but I'm not sure if locally attributing kinds to sourcekitd's refactor ids are a good idea, so for now everything is marked as the .refactor kind. I was thinking a better approach would be to make sourcekitd return a lsp_refactor_id string and use that internally.

/// The document in which the command was invoked.
public var textDocument: TextDocumentIdentifier

public init(range: PositionRange, context: CodeActionContext, textDocument: TextDocumentIdentifier) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm trying to minimize use of PositionRange, so I suggest making the initializer take Range<Position> and convert it. The only reason we have PositionRange is to make the serialization work. Ideally it would just be an implementation detail, but for now it's part of the API so that we can keep getting Codable synthesis.

@benlangmuir
Copy link
Contributor

This conflicts with #19 due to the codeAction addition, but it's gonna take some time for me to get this fully working so the conflict could be gone by then.

I'd be happy to take the changes for adding the CodeAction API as its own PR.

@rockbruno
Copy link
Contributor Author

I like that idea Ben, quite a few features rely on it.

@rockbruno
Copy link
Contributor Author

I'm separating this into three CodeAction/ExecuteCommand/Refactoring PRs because I feel that the second one is going to need some special attention. I should have the skeleton of CodeActions ready soon.

@rockbruno
Copy link
Contributor Author

rockbruno commented Jan 20, 2019

This is fully working now, I just need to clean it up and write some tests.

There are three "bad" news though:

  • Although VSCode supports name placeholder snippets, it looks like the LSP only supports them for completion requests... I tried to find a way to trigger them from the refactoring requests but haven't found it yet, so currently you have to edit the new method/property names by hand. Text Snippets not available for CodeAction microsoft/language-server-protocol#592
  • It looks like sourcekitd doesn't add tabs/spaces to the first line of each edit, so you end up with this:
fileprivate func extractedFunc4() -> Int {
var a = 3
  a = a + 1
  return 1
}
key.sourcetext: "fileprivate func extractedFunc() {\nlet asd = 1\n    let asd = 1\n    let asd = 1\n    let asd = 1\n}\n\n",

But that's fixable from our side so it's not that bad. I wonder if Xcode doesn't suffer from this because it formats the range after editing.

@rockbruno rockbruno changed the title [WIP] Add Local Refactoring support Add Local Refactoring support Jan 26, 2019
@rockbruno rockbruno force-pushed the local-refactoring branch 2 times, most recently from c6e1a7c to bd1eb85 Compare June 28, 2019 00:22
Tests

READMË

import Foundation

/// Request sent from the client to to trigger command execution on the server.
Copy link
Contributor

@literalpie literalpie Aug 7, 2019

Choose a reason for hiding this comment

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

There is a duplicate "to" here

@rockbruno
Copy link
Contributor Author

Lots of things happened since this one, so I'll open a new PR with only the info that ended up mattering.

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.

3 participants