[#2118] Implement keyword snippets for control-flow keywords (if/for/while/etc.) #2387
Conversation
DPrakashh
requested review from
ahoppen,
bnbarham,
hamishknight and
rintaro
as code owners
|
Hi @ahoppen , @bnbarham , @hamishknight , @rintaro Hope you're having a good week. I wanted to check on the status of my PR from four days ago. I know these core completion changes take time to review carefully. Thanks for your time! |
ahoppen
left a comment
ahoppen
left a comment
There was a problem hiding this comment.
Thank you. The overall structure looks good, I have left a few comments inline.
Also, do you have a screen recording or something like that how this looks like in VS Code? Ie. point VS Code to a locally built version of SourceKit-LSP as described in https://github.com/swiftlang/sourcekit-lsp/blob/main/CONTRIBUTING.md#using-a-locally-built-sourcekit-lsp-in-an-editor.
|
|
||
| switch keyword { | ||
| case "if": | ||
| return "if ${1:condition} {\n\t${0:}\n}" |
There was a problem hiding this comment.
We should not assume that the source file is indented with tabs. We have BasicFormat.inferIndenatation to infer a source files’s indentation and should rely and that instead.
| @@ -0,0 +1,247 @@ | |||
| @_spi(SourceKitLSP) import LanguageServerProtocol | |||
There was a problem hiding this comment.
Could you add the copyright header to this file?
|
|
||
| // Check if this is a keyword that should be converted to a snippet | ||
| var isKeywordSnippet = false | ||
| if completionKind == .keyword, let snippetText = keywordSnippet(for: name) { |
There was a problem hiding this comment.
If this evaluates to true, we can avoid computing the previous textEdit. Since almost all parameters are the same, it should even be sufficient to just use snippetText for text and just have a single call to computeCompletionTextEdit.
| filterText: filterName, | ||
| insertText: text, | ||
| insertTextFormat: isInsertTextSnippet ? .snippet : .plain, | ||
| insertText: insertText, |
There was a problem hiding this comment.
I don’t think this is correct. text will have SourceKit placeholders replaced by LSP placeholders while insertText still contains SourceKit placeholders. I expect that we have tests that fail with this change. Did you run all tests locally?
| guard let ifItem = completions.items.first(where: { $0.label == "if" }) else { | ||
| XCTFail("No completion item with label 'if'") | ||
| return | ||
| } |
There was a problem hiding this comment.
| guard let ifItem = completions.items.first(where: { $0.label == "if" }) else { | |
| XCTFail("No completion item with label 'if'") | |
| return | |
| } | |
| let ifItem = try XCTUnwrap(completions.items.first(where: { $0.label == "if" })) |
Same in a couple more places below
|
@ahoppen on it sir |
2 participants
This PR implements snippet completions for control-flow keywords (if, for, while, guard, switch, repeat) when they are used in code completion.
The solution involved updating "CodeCompletionSession.swift" to identify keyword completions (.keyword) and overwrite their "insertText" and "textEdit" with the correct LSP snippet strings, setting the insertTextFormat to .snippet.
This ensures that keywords now contain placeholders (e.g.,
${1:condition}and$0) required by LSP clients.Verification
New unit tests were added to "SwiftCompletionSnippetTests.swift" to explicitly verify:
$1,$0).All local tests, including the focused test (
swift test --filter SwiftCompletionSnippetTests/testKeywordIfProvidesSnippet), passed successfully.Checklist (Based on Contributing Guide)
1-Code has been formatted using `swift format -ipr
2-New tests were added to cover the feature.
Fixes #2118