Skip to content
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 12 additions & 0 deletions exe/ruby-lsp
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,13 @@ parser = OptionParser.new do |opts|
options[:branch] = branch
end

opts.on(
"--path [PATH]",
Copy link
Member

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?

"Launch the Ruby LSP using a local PATH rather than the release version",
) do |path|
options[:path] = path
end

opts.on("--doctor", "Run troubleshooting steps") do
options[:doctor] = true
end
Expand All @@ -54,6 +61,11 @@ rescue OptionParser::InvalidOption => e
exit(1)
end

if options[:branch] && options[:path]
warn('Invalid options: --branch and --path cannot both be set.')
exit(1)
end

# When we're running without bundler, then we need to make sure the composed bundle is fully configured and re-execute
# using `BUNDLE_GEMFILE=.ruby-lsp/Gemfile bundle exec ruby-lsp` so that we have access to the gems that are a part of
# the application's bundle
Expand Down
14 changes: 14 additions & 0 deletions jekyll/contributing.markdown
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,20 @@ with a debugger. Note that the debug mode applies only until the editor is close
Caveat: since you are debugging the language server instance that is currently running in your own editor, features will
not be available if the execution is currently suspended at a breakpoint.

#### Configuring the server version

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.
Copy link
Member

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.

Suggested change
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.


```jsonc
{
"rubyLsp.serverPath": "/path/to/your/ruby-lsp-clone"
}
```

*Note: There are some exceptions to this. Most notably, when the ruby-lsp repository is opened in VS Code with the extension active, it will run the server contained within the repository.

#### Understanding Prism ASTs

The Ruby LSP uses Prism to parse and understand Ruby code. When working on a feature, it's very common to need to
Expand Down
14 changes: 13 additions & 1 deletion lib/ruby_lsp/setup_bundler.rb
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,13 @@ def stdout
def initialize(project_path, **options)
@project_path = project_path
@branch = options[:branch] #: String?
@path = options[:path] #: String?
@launcher = options[:launcher] #: bool?

if @branch && [email protected]? && @path && [email protected]?
Copy link
Member

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.

raise ArgumentError, "Branch and path options are mutually exclusive. Please specify only one."
end

patch_thor_to_print_progress_to_stderr! if @launcher

# Regular bundle paths
Expand Down Expand Up @@ -165,7 +171,13 @@ def write_custom_gemfile

unless @dependencies["ruby-lsp"]
ruby_lsp_entry = +'gem "ruby-lsp", require: false, group: :development'
ruby_lsp_entry << ", github: \"Shopify/ruby-lsp\", branch: \"#{@branch}\"" if @branch
if @branch && [email protected]?
ruby_lsp_entry << ", github: \"Shopify/ruby-lsp\", branch: \"#{@branch}\""
end
if @path && [email protected]?
absolute_path = File.expand_path(@path, @project_path)
Copy link
Member

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.

ruby_lsp_entry << ", path: \"#{absolute_path}\""
end
parts << ruby_lsp_entry
end

Expand Down
32 changes: 32 additions & 0 deletions test/setup_bundler_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -280,6 +280,38 @@ def test_creates_composed_bundle_with_specified_branch
end
end

def test_creates_composed_bundle_with_specified_path
Dir.mktmpdir do |dir|
local_path = "local-ruby-lsp"
FileUtils.mkdir_p(File.join(dir, local_path, "lib"))

Dir.chdir(dir) do
bundle_gemfile = Pathname.new(".ruby-lsp").expand_path(Dir.pwd) + "Gemfile"
Bundler.with_unbundled_env do
stub_bundle_with_env(bundle_env(dir, bundle_gemfile.to_s))
run_script(File.realpath(dir), path: local_path)
end

assert_path_exists(".ruby-lsp")
assert_path_exists(".ruby-lsp/Gemfile")
expected_absolute_path = File.expand_path(local_path, File.realpath(dir))
assert_match(%r{ruby-lsp.*path: "#{Regexp.escape(expected_absolute_path)}"}, File.read(".ruby-lsp/Gemfile"))
assert_match("debug", File.read(".ruby-lsp/Gemfile"))
end
end
end

def test_raises_error_when_both_branch_and_path_are_specified
Dir.mktmpdir do |dir|
Dir.chdir(dir) do
error = assert_raises(ArgumentError) do
RubyLsp::SetupBundler.new(dir, branch: "test-branch", path: "local-path")
end
assert_equal("Branch and path options are mutually exclusive. Please specify only one.", error.message)
end
end
end

def test_returns_bundle_app_config_if_there_is_local_config
Dir.mktmpdir do |dir|
Dir.chdir(dir) do
Expand Down
5 changes: 5 additions & 0 deletions vscode/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -467,6 +467,11 @@
"type": "string",
"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",
Copy link
Member

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.

"type": "string",
"default": ""
Copy link
Member

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.

},
"rubyLsp.pullDiagnosticsOn": {
"description": "When to pull diagnostics from the server (on change, save or both). Selecting 'save' may significantly improve performance on large files",
"type": "string",
Expand Down
39 changes: 34 additions & 5 deletions vscode/src/client.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import path from "path";
import fs from "fs";
import os from "os";
import { performance as Perf } from "perf_hooks";

Expand Down Expand Up @@ -85,6 +86,7 @@ function getLspExecutables(workspaceFolder: vscode.WorkspaceFolder, env: NodeJS.
const customBundleGemfile: string = config.get("bundleGemfile")!;
const useBundlerCompose: boolean = config.get("useBundlerCompose")!;
const bypassTypechecker: boolean = config.get("bypassTypechecker")!;
const serverPath: string = config.get("serverPath")!;

const executableOptions: ExecutableOptions = {
cwd: workspaceFolder.uri.fsPath,
Expand Down Expand Up @@ -119,13 +121,40 @@ function getLspExecutables(workspaceFolder: vscode.WorkspaceFolder, env: NodeJS.
options: executableOptions,
};
} else {
const args = [];
const workspacePath = workspaceFolder.uri.fsPath;
const command =
path.basename(workspacePath) === "ruby-lsp" && os.platform() !== "win32"
? path.join(workspacePath, "exe", "ruby-lsp")
: "ruby-lsp";
let command: string;

const args = [];
if (serverPath.length > 0 && branch.length > 0) {
Copy link
Member

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.

throw new Error(
'Invalid configuration: "rubyLsp.serverPath" and "rubyLsp.branch" cannot both be set. Please unset one of them.',
);
}

if (serverPath.length > 0) {
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}". `,
);
}
Copy link
Member

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 through vscode.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.

Copy link
Contributor Author

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.

} else {
command =
path.basename(workspacePath) === "ruby-lsp" && os.platform() !== "win32"
? path.join(workspacePath, "exe", "ruby-lsp")
: "ruby-lsp";
}

if (branch.length > 0) {
args.push("--branch", branch);
Expand Down