Skip to content

Use synchronize-request in LSP tests on 6.2 #1461

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

Merged
merged 1 commit into from
Mar 25, 2025

Conversation

plemarquand
Copy link
Contributor

The tests used workspace/_pollIndex to determine if sourcekit-lsp had finished indexing, however this hidden request was removed from sourcekit-lsp recently in favour of workspace/_synchronize

Issue: #1457

The tests used `workspace/_pollIndex` to determine if sourcekit-lsp had
finished indexing, however this hidden request was removed from
sourcekit-lsp recently in favour of `workspace/_synchronize`

Issue: swiftlang#1457
@plemarquand
Copy link
Contributor Author

cc @ahoppen

@plemarquand plemarquand merged commit 61f8df7 into swiftlang:main Mar 25, 2025
13 of 15 checks passed
Comment on lines +50 to +53
"swift.sourcekit-lsp.serverArguments": [
"--experimental-feature",
"synchronize-request",
],
Copy link
Member

Choose a reason for hiding this comment

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

Instead of adding the to specify this as part of initializationOptions during the initialize request? As a general direction, I would like to move away from taking command line arguments to SourceKit-LSP.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What does that look like? Adding "workspace/synchronize-request": true to the options here? https://github.com/swiftlang/vscode-swift/blob/main/src/sourcekit-lsp/LanguageClientManager.ts#L650

Copy link
Member

Choose a reason for hiding this comment

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

It should be adding "experimentalFeatures": ["synchronize-request"] there. Though it would be nice to only enable that experimental feature for tests that need it. If that’s not possible, let me know and we can try to prioritize making the request non-experimental.

Copy link
Contributor Author

@plemarquand plemarquand Mar 26, 2025

Choose a reason for hiding this comment

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

Thanks, I've addressed this in #1463

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.

4 participants