Skip to content
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

Auto detect formatter when client did not specify it #723

Merged
merged 3 commits into from
May 30, 2023

Conversation

asok
Copy link
Contributor

@asok asok commented May 20, 2023

Motivation

I was playing with ruby-lsp and Emacs client eglot and I noticed that formatting of the buffer is not working. It seems that the client is not passing the "auto" formatter in the initializationOptions.

I'm very green to lsp so I don't quite understand everything. But when following this thread I've read that the server should apply defaults by itself:

a server should work with an empty literal as well and simply assume a set of defaults

Implementation

Assume by default that the formatter should be auto detected if the initialization option is missing.

Automated Tests

No automated tests yet since I'm opening a draft pull request in order to find out if it's the right way to proceed.

Manual Tests

  • Install Emacs 26.3+
  • create ~/.emacs.d/init.el file with contents:
  (require 'package)
  (add-to-list 'package-archives '("melpa" . "https://melpa.org/packages/") t)
  (add-to-list 'package-archives '("gnu-devel" . "https://elpa.gnu.org/devel/") t)
  (package-initialize))

  (package-install "eglot")
  (require 'eglot)
  (add-to-list 'eglot-server-programs '((ruby-mode ruby-ts-mode) "ruby-lsp"))
  • open a ruby file in Emacs
  • run eglot command in Emacs
  • run eglot-format-buffer command in Emacs, you should see the error message "Unknown formatter: " in the minibuffer
  • apply the patch to your ruby-lsp gem
  • run eglot-reconnect command in Emacs
  • run eglot-format-buffer command in Emacs, you should not see the error

Copy link
Member

@vinistock vinistock left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed, the server should always set it's defaults internally if nothing is received.

Can you please sign the CLA?

@@ -421,7 +421,7 @@ def initialize_request(options)
end

formatter = options.dig(:initializationOptions, :formatter)
@store.formatter = if formatter == "auto"
@store.formatter = if formatter == "auto" || formatter == '' || formatter.nil?
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there any scenario where formatter would be an empty string instead of nil?

I don't believe there would be. If it can only be nil, let's move this up instead and do

formatter = options.dig(:initializationOptions, :formatter) || "auto"

@asok
Copy link
Contributor Author

asok commented May 26, 2023

I have signed the CLA!

@asok asok marked this pull request as ready for review May 26, 2023 22:14
@asok asok requested a review from a team as a code owner May 26, 2023 22:14
@asok asok requested review from KaanOzkan and vinistock May 26, 2023 22:14
@andyw8 andyw8 added the enhancement New feature or request label May 29, 2023
@andyw8
Copy link
Contributor

andyw8 commented May 29, 2023

@asok there's a minor linting issue to address.

@vinistock vinistock merged commit 9095d4d into Shopify:main May 30, 2023
@vinistock
Copy link
Member

Thanks for the contribution! CI is failing, but it's unrelated to these changes.

@asok asok deleted the handle-blank-formatter branch May 30, 2023 18:48
@shopify-shipit shopify-shipit bot temporarily deployed to production June 16, 2023 18:06 Inactive
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants