Cleanup extension #106
Merged
Cleanup extension #106
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Draft until #105 |
Adds a config option to specify a specific command to run the language server. When not set, the extension falls back to running with coursier. By default, the config option is not set. I need to clean up the config option a bit, since when you specify a specific command, you can just use stdio, meaning the arguments passed to the language server should just be "0". So really no args should be set in the config option.
The language server tells vscode it provides a formatter through its capabilities, so we don't need any client-side code.
Cleaned up the implementations and organization of the smithy/jarFileContents and smithy/selectorCommand lsp extension handlers.
Refactored language client options and server config sent on initialization, and removed unnecessary config. We had some logic to figure out the root directory and workspace folder which was used to refine which files the extension should care about, but I think this is now unnecessary because the language server will just find project roots anyway. There was also an extension config option to set the root path, which should be unnecessary for the same reason, so I removed it. The client still sends the workspace folder and root path in the initialize request, so we aren't losing any information here. I also removed the file system watcher, as the server will dynamically register that.
Two things: Some refactoring to use execFile instead of spawn for checking if coursier is available, which is more lightweight. Changed the way the server is run with coursier to have vscode run the coursier command directly. Previously, we created a local tcp server that acted as a proxy for the language server, which we ran in a separate process. But I don't think this extra layer of indirection is necessary, since vscode's lsp api can be given a command and some args to use to start the server.
Changed the prefix of config properties from `smithyLsp` to just `smithy` (or `smithy.server`, where applicable). I kept the existing config properties, but added a deprecation message to them, pointing to the new property. We will keep these in the next release, but remove them in the following release. Small note about the `smithy.server.version` property: I didn't give it a default value, since that would have been chosen over a user-defined version in the old property. When we remove the old properties, we can add the default to the new one. I also added a simple api for getting config settings in the extension, that prefers the new properties over the old.
In a previous commit, I changed the server initializationOptions to have a nested `diagnostics` object. That isn't what the server expects at the moment, and it is probably more complicated than necessary to change, so I changed it back.
For some reason, when a string config option isn't specified it gets a default value of `""`, so when we checked the new config option, the empty string value would get chosen instead. So I added a default of null for the old options, and I also switched the existing defaults to the new options, choosing the old options first if they're present.
Not sure why I removed it in the first place.
milesziemer
force-pushed
the
cleanup-extension
branch
from
ec13a7a to
6fcb681
Compare
milesziemer
requested review from
JordonPhillips and
joewyz
and removed request for
JordonPhillips
joewyz
approved these changes
Apr 7, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR includes a number of various cleanups and additions.
if the language server advertises the capability, which it does.
smithy/jarFileContentsandsmithy/selectorCommand.root directory is, or send it to the language server, since vscode and the language server will infer it. I also
removed the
rootPathconfig option for the same reasons. We also set up a client-side file system watcher, whichis unnecessary since the server will dynamically register it.
server running in a separate process which we ran manually. This is unnecessary because the vscode extension API
allows you to provide a command + args that it will use to run the language server. This at the very least improves
startup time.
for each existing option with the prefix
smithy(orsmithy.serverwhere applicable), instead of thesmithyLspprefix. I kept the old options, but deprecated them, and removed their default values. The extension will now choose
the old options when they've been specified, but defaults to the new options. In a following release, we will remove
the old options.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.