ruby-lsp icon indicating copy to clipboard operation
ruby-lsp copied to clipboard

Allow non-standard values for rubyLsp.formatter option

Open jscharf opened this issue 2 years ago • 7 comments

This feature is VS Code specific

  • [X] VS Code specific

Use case

While working on a new addon last week, I came across this warning

Value is not accepted. Valid values: "auto", "rubocop", "syntax_tree", "none".(1)
image

I think we should be able to add new values in there without having a warning. This will make it less confusing for potential addon creators that their new value is acceptable and not going to cause a problem.

Description

I heard that the value is currently an Enum, so this feature could potentially be just removing the Enum.

However there may be an opportunity to do something more.

Implementation

If it's possible (I don't know if it is), we could have the list be the current list of "auto", "rubocop", "syntax_tree", "none" but then also include any known registered formatters.

In the activate method of the addon, we run this type of code

RubyLsp::Requests::Formatting.register_formatter("rubyfmt", RubyFmtFormatterRunner.instance)

So is it possible that we can detect which formatters have been registered to the system?

If so, we could make the list equal the list of known registered formatters.

Let me know what you think!

jscharf avatar Nov 21 '23 14:11 jscharf

Thank you for the feature suggestion!

Unfortunately, there's no way to read the enum entries programatically as far as I know. We declare the options in the package.json and whatever is in there is valid.

So I don't think we'd be able to read what addons are available in any way. The decision we'd need to make is:

  • Do we want to allow for any string, which removes validation, but also removes the auto-complete when selecting
  • Do we want to add more formatters to the list

I think it's fine to add more formatters to the list. It keeps the validation and completion. There aren't that many formatters out there, so I think it should be fine.

Would you like to put a PR up adding rubyfmt to the list?

vinistock avatar Nov 21 '23 15:11 vinistock

🤔 A downside of adding other formatters is that it may give the impression they are natively supported, when they actually need an addon.

andyw8 avatar Nov 21 '23 18:11 andyw8

That's a fair point. We could display an information message from the server if it is initialized with a formatter and it cannot find it in the registry.

vinistock avatar Nov 21 '23 19:11 vinistock

I would love to open a PR to add rubyfmt to the list!

I do agree with @andyw8's point about it being a UX issue though. When I see the list pop up (autocomplete) while editing the JSON file, I assume that any of those options will work "out of the box". So if needs to stay as an enum, I think having a message is a good idea. When you say "information message" are these the messages that appear in the bottom right corner in the UI?

jscharf avatar Nov 21 '23 20:11 jscharf

Yes. The server can push dialogues for the editor to display through notifications. Here we show an error is there's a problem with formatting.

What we'd need to do is after loading addons is complete here, we check if the registry includes the formatter saved in the store (example). If it does not, we push a window/showMessage notification warning the user. Probably as a warning and not an error dialogue though.

vinistock avatar Nov 21 '23 21:11 vinistock

Sounds good, thanks for the extra information. I've opened https://github.com/Shopify/ruby-lsp/issues/1213 to track the issue on the ruby-lsp project since it sounds like the work is going to be done there.

jscharf avatar Nov 24 '23 15:11 jscharf

This issue is being marked as stale because there was no activity in the last 2 months

github-actions[bot] avatar Jan 24 '24 12:01 github-actions[bot]

https://github.com/Shopify/ruby-lsp/pull/2092

andyw8 avatar May 27 '24 19:05 andyw8