Use generic method lsp-initialization-options when :initialization-options is nil

[MaskRay]

No description provided.

[vibhavp]

Could you add an example of its usage to the API document?

[yyoncho]

This will break lsp-haskell and lsp-java. Can we keep the initialization-option related functionality until all clients are ported? Why did you delete the go configuration, should you port it to the new mechanism similar to what you did with cquery?

[MaskRay]

The examples can be found in the PRs I created for emacs-ccls, emacs-cquery and lsp-haskell.
I think you'd know how to do it for lsp-java :)

Since I noticed that the default language server has been changed from go-langserver to bingo, and I feel the latter may not need the initialization options. Anyway, some of the configuration may be redundant and it gives the user largest flexibility if it doesn't forward every option. As there might be options not covered by the current defined set. Now, for other lsp-clients servers, the user can freely add their cl-defmethod to customize the initialization options and it is also convenient to customize it per project.

[yyoncho]

@MaskRay

I am fine with the approach, my concern is that we might break someone's config. Here it is a PR #658 from 6 days ago which fixes the bingo server's settings so it seems the configuration used by the lsp-mode users so it is not fine to delete it without any replacement.

(cc @fjl )

I the future, I believe we should create a generic mechanism to handle these settings instead of requring manual creation of defcustoms. E. g. (lsp-defsettings ccls :description "Description" :default "foo" :key "path") which will do the rest of the work and it wont require creation of lsp-initialization-options methods.

[MaskRay]

Yes, it may break, as the current approach forward every recognized initialization option. #658 was probably based on the assumption that this was the least disruptive change which would made @fjl's own workflow work. I feel deleting it gives the most flexibility. Users can customize initialization options this way:

(cl-defmethod lsp-initialization-options ((_server (eql lsp-clients-go)))
  `(...))

lsp-defsettings

When the need comes, the convenient method may be helpful.

[yyoncho]

@MaskRay are you saying that we are going to delete the defcustom and require user to write a elisp function to customize their server instead of doing customize-variable and set what they want? My understanding from the cquery PR was that we will only change the way initialization function is dispatched and that's why I cannot understand why we delete the go configuration but preserve the cquery/haskell one.

I believe that we should use defcustom on top of language servers settings so each of the settings is discoverable via customize-group/customize-variable which is in the spirit of Emacs.

Let other members share their opinion too.

[MaskRay]

@yyoncho Except lsp-clients-go, other clients defined in lsp-clients.el do not allow users to customize initialization options now. With this change, they'll be able to customize it with (cl-defmethod lsp-initialization-options ((_server (eql lsp-clients-foobar))).

If a client thinks providing a defcustom variable is more convenient, it can define a cl-defcustom which just returns that defcustom variable.

[yyoncho]

@MaskRay you can still customize any of the clients even now with.

(setf (lsp--client-initialization-options (gethash 'ccls lsp-clients))  options...)

The reasons for having customization only for go are:

1. The server does not have customization
2. The users of the language server have not requested customizations.

If a client thinks providing a defcustom variable is more convenient, it can define a cl-defcustom which just returns that defcustom variable.

That is what has happened for lsp-java/lsp-haskell/lsp-go/cquery and I still cannot understand why we remove that option only for golang. I believe that we should always wrap them in defcustom and never require the user to write a custom function except if he wants to do something uncommon.

[hpdeifel]

In the hope that it might be useful, I want to share my ideal workflow for server configuration:

My priorities in order:

1. I want to set some options globally in my .emacs and some in dir-local variables per project. For both of these I would strongly prefer to just set variables and to not execute any other code (dir-local eval is evil). Ideally, variables would be marked as safe when appropriate.
2. I want to be able to discover options from emacs, without reading the server's documentation. Defcustoms are perfect for this.
3. If my lsp-client misses a defcustom for a setting that the server supports, I want to still be able to set it, ideally via .dir-locals.el as above.

[yyoncho]

@hpdeifel we are pretty much at this point except 3) which might be an overkill since it could be replaced with "Do a PR and improve lsp-mode for the other users as well". In dap-mode I am considering directly consuming the vscode extension which directly comes with its configuration. I wonder whether we could go in this direction in lsp-mode.

[MaskRay]

Sorry for getting back to this late. Switched to a backward-compatible approach.

[yyoncho]

I am afraid that at this point, the approach with the generics will limit our possibilities - for example when we define a new client and we use the old client as a template, the code will break since the handlers are no longer packed with the client definitions. For example, in lsp-docker we copy the existing client and change the command and serve id: - https://github.com/emacs-lsp/lsp-docker/blob/master/lsp-docker.el#L53 . WDYT?

[MaskRay]

Emm. I don't understand that. If :initialization-options is not nil, defgeneric will not be used and this change should be a no-op.

[yyoncho]

My point is that we now depend on the fact that if we do copy-lsp--client, change the server-id and register the client it will work fine. This will break if we start doing dynamic dispatch via cl-defmethod since dispatch is performed by server-id.

[yyoncho]

To elaborate a bit more: if we decide to go this way we have to find a way to deeply copy the methods that are attached via cl-defmethod and attach them to the new definition.