standardrb / standard

Ruby's bikeshed-proof linter and formatter 🚲
Other
2.73k stars 214 forks source link

Add support for Ruby LSP as a built-in add-on #630

Closed searls closed 5 months ago

searls commented 6 months ago

Release plan

Background

After talking to @vinistock at Kaigi, I want to take a stab at advertising a working Ruby LSP addon from within the gem so that Ruby LSP users can get standard support without running the LSP themselves (or installing a custom extension, like our VS Code extension

I'm trying and failing to get this end-to-end working in an example app, so I'm trying to take a step back and just get this test passing. The result's response is nil, but (one assumes?) it shouldn't be. More importantly, WrapsBuiltinLspStandardizer#run_formatting isn't being invoked, so my attempts to set the formatter to `standard is probably not working, either.

To replicate the failure, you should be able to run:

$ bundle
$ bundle exec m test/ruby_lsp_addon_test.rb
st0012 commented 6 months ago

It's failing because Ruby LSP doesn't format files outside of the workspace path. You can workaround that with:

-    with_server(source) do |server, uri|
+    with_server(source) do |server, _uri|
+      uri = URI(File.join(server.global_state.workspace_path, 'fake.rb'))
       server.process_message(
         id: 1,
-        method: "textDocument/formatting",
-        params: {textDocument: {uri: uri}, position: {line: 0, character: 0}}
+        method: 'textDocument/formatting',
+        params: { textDocument: { uri: uri },
+                  position: { line: 0, character: 0 } }
       )

However, it'd then actually try to run formatting with that file, which will result to another error since the file doesn't actually exist.

So you'll likely need additional setup to:

searls commented 6 months ago

@st0012 thanks Stan! Got through that and have it formatting correctly now.

Question: can I safely rely on Dir.pwd being the workspace directory, as I do with my custom extension?

Question: this test is now failing b/c the range being returned ends on line 19, column 19, even though the source string is only 2 lines long and that second line is only 5 or 6 characters. Since the addon is not providing the range, is it a bug?

searls commented 6 months ago

I'm unsure the cause of the test failure this introduces, but just looking at the addon stuff, does this look right? @vinistock, @st0012?

searls commented 6 months ago

Looking into this failure, I have very few good answers.

AFAICT, if one executes require "ruby-lsp/internal" in our test suite, then the CLI test will start failing, with the odd behavior of suddenly being incapable of autofixing Lint/TrailingCommaInAttributeDeclaration

The test will print this to stderr:

tmp/cli_test/subject.rb:18:19: Lint/TrailingCommaInAttributeDeclaration: Avoid leaving a trailing comma in attribute declarations.

And then the test will fail because the shelled-out run will exit non-zero.

This is a bit of a mystery to me. Like, the best explanation I can imagine is that loading ruby_lsp/internal causes some Rubocop something-or-other to be loaded which in turn mutates the default configuration set of Lint/TrailingCommaInAttributeDeclaration so as to flag it as not autocorrectable?

Extremely odd failure

searls commented 6 months ago

Hey @vinistock @st0012, I'm trying to validate this actually works by setting up a project pointing at my local copy using:

  gem "standard", path: "/path/to/standardrb/standard"

But Ruby LSP doesn't seem to pick it up as a formatter/linter option.

screenshot-2024-05-23-11h16m16s

(If I puts something at the end of the addon's activate method, I'll see it blow up in the Ruby LSP output tab, so I know it's being required, however)

Question: do you have any idea why this might not be working when the plugin is a local path-sourced gem?

Meanwhile, if I change my dependency to look at this github branch:

gem "standard", github: "standardrb/standard", branch: "ruby-lsp-spike"

And then I check the output tab of VS Code, I get a stack trace without an error message:

2024-05-23 11:30:02.677 [info] (grog) /Users/justin/.rbenv/versions/3.3.0/lib/ruby/gems/3.3.0/bundler/gems/standard-e7d5e49aafd2/lib/standard/plugin/creates_runner_context.rb:7:in `call'

/Users/justin/.rbenv/versions/3.3.0/lib/ruby/gems/3.3.0/bundler/gems/standard-e7d5e49aafd2/lib/standard/plugin/merges_plugins_into_rubocop_config.rb:29:in `call'

/Users/justin/.rbenv/versions/3.3.0/lib/ruby/gems/3.3.0/bundler/gems/standard-e7d5e49aafd2/lib/standard/plugin/combines_plugin_configs.rb:11:in `call'

/Users/justin/.rbenv/versions/3.3.0/lib/ruby/gems/3.3.0/bundler/gems/standard-e7d5e49aafd2/lib/standard/creates_config_store.rb:22:in `block in call'

<internal:kernel>:90:in `tap'

/Users/justin/.rbenv/versions/3.3.0/lib/ruby/gems/3.3.0/bundler/gems/standard-e7d5e49aafd2/lib/standard/creates_config_store.rb:19:in `call'

/Users/justin/.rbenv/versions/3.3.0/lib/ruby/gems/3.3.0/bundler/gems/standard-e7d5e49aafd2/lib/standard/builds_config.rb:32:in `call'

/Users/justin/.rbenv/versions/3.3.0/lib/ruby/gems/3.3.0/bundler/gems/standard-e7d5e49aafd2/lib/ruby_lsp/standard/wraps_built_in_lsp_standardizer.rb:15:in `init!'

/Users/justin/.rbenv/versions/3.3.0/lib/ruby/gems/3.3.0/bundler/gems/standard-e7d5e49aafd2/lib/ruby_lsp/standard/wraps_built_in_lsp_standardizer.rb:11:in `initialize'

/Users/justin/.rbenv/versions/3.3.0/lib/ruby/gems/3.3.0/bundler/gems/standard-e7d5e49aafd2/lib/ruby_lsp/standard/addon.rb:15:in `new'

/Users/justin/.rbenv/versions/3.3.0/lib/ruby/gems/3.3.0/bundler/gems/standard-e7d5e49aafd2/lib/ruby_lsp/standard/addon.rb:15:in `activate'

So I'm kind of at a loss as to how to actually test the plugin locally, as a result.

st0012 commented 6 months ago

It looks like the error was caused by Standard::VERSION not being defined when it's called here. Adding require_relative "../../standard/version" makes everything works as expected.

Screenshot 2024-05-23 at 22 31 16

And for some reason the addon's error reporting mechanism didn't work as expected as the error message is not displayed. I'll give it a look when I'm back to work.

andyw8 commented 6 months ago

And for some reason the addon's error reporting mechanism didn't work as expected as the error message is not displayed.

This should be solved by https://github.com/Shopify/ruby-lsp/pull/2085

searls commented 6 months ago

Thank you both for the above. I fixed the constant not defined error, but even after clearing it I'm seeing the same behavior where the plugin isn't recognized as a formatter

Setting value of "rubyLsp.formatter" to "standard" yields the problem:

> Value is not accepted. Valid values: "auto", "rubocop", "syntax_tree", "none".

Additionally, when I choose "format document" from the command pallete, I get "There is no formatter for 'ruby' files installed."

Here's a sample repo to demo the config: https://github.com/searls/ruby-lsp-standard-example

andyw8 commented 6 months ago

@searls You probably need to add standard to the enum here.

But the extension shouldn't really have to know about specific addons, so we probably need to re-think this approach.

searls commented 6 months ago

@andyw8 ahh, yes, if it's just hard-coded now that won't really scale to third-party add-ons. I was assuming the extension was phoning home to the LSP and then updating the configuration schema dynamically (somehow? Think I was conflating it with WorkspaceConfiguration API, which I doubt can be used for that purpose.)

It might be necessary to just relax that configuration property in the schema to allow it to be set to any value?

andyw8 commented 6 months ago

Looking into some options: https://github.com/Shopify/ruby-lsp/pull/2092

andyw8 commented 5 months ago

@searls standard is now available as a formatter option: https://github.com/Shopify/ruby-lsp/pull/2092

searls commented 5 months ago

[UPDATE: Nevermind, got it running locally OK. Forgot I had to enable the format & diagnostic features on rubyLsp.enabledFeatures]

@andyw8 is there any way I ought to be able to test this now?

I tried pointing "rubyLsp.branch": "main" since the gem hasn't been released since you merged https://github.com/Shopify/ruby-lsp/pull/2092, but of course that's based on the vscode extension's package.json, so then I cloned and ran that in a host extension but, try as I might, even though I can get the following config to clear warnings/errors:

{
  "[ruby]": {
    "editor.defaultFormatter": "Shopify.ruby-lsp"
  },
  "rubyLsp.formatter": "standard",
  "rubyLsp.linters": [
    "standard"
  ]
}

… I don't get any diagnostics and I still get "There is no formatter for 'ruby' files installed." when I try to manually invoke the Format Document command

searls commented 5 months ago

Just released this basic version (without code actions) as v1.38.0

searls commented 5 months ago

@vinistock @andyw8 if you would be so kind, let me know when a version of the vscode extension has shipped with the standard enum present and I'll go to town testing this more thoroughly, adding it to our docs, and so forth

andyw8 commented 5 months ago

@searls you can test it by checking out the main branch, selecting the vscode workspace and, then selecting Run and Debug:

https://github.com/Shopify/ruby-lsp/blob/main/CONTRIBUTING.md#live-debugging

Everyone on the team is at a company event this week, so there may be a little delay until the next release.

searls commented 5 months ago

Heads up I also merged #636 which adds Code Action support (and duplicates half of Ruby LSP's rubocop integration in the process)

andyw8 commented 4 months ago

@vinistock @andyw8 if you would be so kind, let me know when a version of the vscode extension has shipped with the standard enum present and I'll go to town testing this more thoroughly, adding it to our docs, and so forth

This shipped today: https://github.com/Shopify/ruby-lsp/releases/tag/vscode-ruby-lsp-v0.7.6

searls commented 4 months ago

Thanks @andyw8. I've verified everything works, and so have updated these docs:

I also just published a blog post that I hope you all will appreciate as supporting the vision that @vinistock and others have laid out for the Ruby LSP project.

Additionally, I'd love if you highlights this addon on your repo's README along with the others, since the fact that this one is built-in will make discovery harder for Ruby LSP users (especially in the presence of our own bespoke LSP server and VS Code plugin).

st0012 commented 4 months ago

I've created https://github.com/Shopify/ruby-lsp/pull/2263 to include standard in the readme 👍

vinistock commented 4 months ago

@searls thank you very much for the blog post and your work on this. Having your support for the vision is really important and I appreciate it 🙏.