-
Notifications
You must be signed in to change notification settings - Fork 218
Add VS Code rubyLsp.serverPath configuration #3713
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
base: main
Are you sure you want to change the base?
Conversation
…ment... Also add --path option to the ruby-lsp executable itself
How to use the Graphite Merge QueueAdd the label graphite-merge to this PR to add it to the merge queue. You must have a Graphite account in order to use the merge queue. Sign up using this link. An organization admin has enabled the Graphite Merge Queue in this repository. Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is great. I left some comments, but the implementation is in the right direction
exe/ruby-lsp
Outdated
end | ||
|
||
opts.on( | ||
"--path [PATH]", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about we call this --lsp-path
to prevent any confusion with a potential ability to configure the workspace path or some other path?
jekyll/contributing.markdown
Outdated
|
||
When developing the Ruby LSP server, you may want to test your changes in your own Ruby projects to ensure they work correctly in real-world scenarios. | ||
|
||
The running server, even in debug mode, will default to the installed release version*. You can use the `rubyLsp.serverPath` configuration setting in the target workspace to start your local copy instead. Make sure to restart the language server after making changes to pick up your updates. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We automatically restart the server when any rubyLsp.
settings change, so you actually don't need a manual restart.
The running server, even in debug mode, will default to the installed release version*. You can use the `rubyLsp.serverPath` configuration setting in the target workspace to start your local copy instead. Make sure to restart the language server after making changes to pick up your updates. | |
The running server, even in debug mode, will default to the installed release version*. You can use the `rubyLsp.serverPath` configuration setting in the target workspace to start your local copy instead. |
lib/ruby_lsp/setup_bundler.rb
Outdated
@path = options[:path] #: String? | ||
@launcher = options[:launcher] #: bool? | ||
|
||
if @branch && !@branch.empty? && @path && !@path.empty? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We're currently handling this both in the executable and here. I think we can skip this validation.
lib/ruby_lsp/setup_bundler.rb
Outdated
ruby_lsp_entry << ", github: \"Shopify/ruby-lsp\", branch: \"#{@branch}\"" | ||
end | ||
if @path && !@path.empty? | ||
absolute_path = File.expand_path(@path, @project_path) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this path expansion correct? The @project_path
is the workspace currently being worked on and @path
is the path to the LSP, which are potentially unrelated.
I think we can set the expectation that the path to the LSP clone must always be an absolute path and if it's not we error early in the executable.
vscode/src/client.ts
Outdated
let command: string; | ||
|
||
const args = []; | ||
if (serverPath.length > 0 && branch.length > 0) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similarly here, I think we can do without this validation since the executable already errors in this case. These specific settings are for the development of the LSP only and don't impact end user experience, so we don't need to worry too much about the failure scenarios.
vscode/package.json
Outdated
"rubyLsp.serverPath": { | ||
"description": "Absolute or workspace-relative path to a local ruby-lsp repository or its ruby-lsp executable. Only supported if not using bundleGemfile", | ||
"type": "string", | ||
"default": "" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can avoid setting a default here, so that when nothing is configured the value is undefined
and we can simply check if (value)
instead of checking for the string's length.
vscode/src/client.ts
Outdated
const absoluteServerPath = path.isAbsolute(serverPath) ? serverPath : path.resolve(workspacePath, serverPath); | ||
const exists = fs.existsSync(absoluteServerPath); | ||
|
||
if (exists) { | ||
args.push("--path", absoluteServerPath); | ||
const stat = fs.statSync(absoluteServerPath); | ||
|
||
if (stat.isDirectory()) { | ||
command = os.platform() !== "win32" ? path.join(absoluteServerPath, "exe", "ruby-lsp") : "ruby-lsp"; | ||
} else { | ||
command = absoluteServerPath; | ||
} | ||
} else { | ||
throw new Error( | ||
`The configured rubyLsp.serverPath "${serverPath}" does not exist at "${absoluteServerPath}". `, | ||
); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few things about this section:
- We don't need to change the command, it will still be
ruby-lsp
, we just need to append the new CLI flag - In VS Code extensions, we cannot ever use
fs
because that bypasses the VS Code file system API. Everything has to happen throughvscode.workspace.fs
That said, since these options are for the development of the LSP itself only, I think we can skip checking these arguments here and move everything to the server (that also ensures that the checks work for any editor connecting to the LSP).
Let's check if the path is absolute and if it exists there and simplify this part.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can't test it without changing the executable path. The extension is running the code with the serverPath configuration but without changing the executable directory, the server won't recognize the new --lsp-path option. I wasn't sure if you meant this, but I also removed the ability to specify the executable directly. I was already on the fence about it and i think it's just unnecessary complexity.
vscode/package.json
Outdated
"default": "" | ||
}, | ||
"rubyLsp.serverPath": { | ||
"description": "Absolute or workspace-relative path to a local ruby-lsp repository or its ruby-lsp executable. Only supported if not using bundleGemfile", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's make this only absolute.
- Rename CLI flag from --path to --lsp-path to prevent confusion with other path configurations - Add validation in ruby-lsp executable to require absolute paths to repo root for --lsp-path option - Do not allow serverPath to directly be an executable - Remove incorrect path expansion in SetupBundler (LSP path and project path are unrelated) - Simplify VS Code extension to pass serverPath directly without validation or fs usage - Remove duplicate validation between executable and SetupBundler - Update package.json to specify absolute path requirement and remove default value - Remove manual restart instruction from documentation (server auto-restarts on config changes) - Fix potential runtime error when serverPath is undefined by using truthy check
Thanks for all of the feedback and good point about the unnecessary validation! I think I addressed everything. I left the validation that makes sure the configured executable path in |
Hey, I'm just checking in on this in case it got lost. Is there anything else that I can do to get this in a good state for review and/or merge? |
Motivation
Closes #3709
Implementation
VS Code Extension
serverPath
configuration setting to package.jsonserverPath
configuration is present ingetLspExecutables
, set--path
as an arg to theruby-lsp
executableRuby LSP Server
--path
arg as an option onruby-lsp
executionsetup_bundler.rb
, add the configured absolute path togem 'ruby-lsp', path: "..."
In every location where the path is considered an error is raised if the branch is also set.
Automated Tests
I followed the test pattern for the
--branch
option when determining what to test. I have added tests for Bundler setup, but not for VS Code nor for option parsing in the ruby-lsp executable. I would be happy to add additional testing if I missed anything.Manual Tests
rubyLsp.serverPath
.serverPath
to your local ruby-lsp repopath:
designation*There are multiple existing issues with the config file watcher and auto-reload when a configuration setting is changed
rubyLsp.branch
norrubyLsp.serverPath