Skip to content

Commit

Permalink
Per #590: Rework README.md about workspace configuration again
Browse files Browse the repository at this point in the history
Also tweak eglot-show-workspace-configuration a bit.

* README.md (Workspace configuration): Rework.

* eglot.el (eglot-show-workspace-configuration): Rework.
(eglot--workspace-configuration-plist): New helper.
  • Loading branch information
joaotavora committed Sep 17, 2022
1 parent 71b9796 commit 18d4627
Show file tree
Hide file tree
Showing 2 changed files with 102 additions and 44 deletions.
103 changes: 77 additions & 26 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -137,41 +137,83 @@ the ensuing TCP connection finds a listening server.
## Workspace configuration

Many servers can guess good defaults and operate nicely
out-of-the-box, but some need to be configured via a special LSP
`workspace/configuration` RPC call to work at all. Additionally, you
may also want a particular server to operate differently across
different projects.
out-of-the-box, but some need to know project-specific settings, which
LSP calls "workspace configuration".

Per-project settings are realized with the Elisp variable
`eglot-workspace-configuration`.
These per-project settings are realized with the Elisp variable
`eglot-workspace-configuration`.

This variable's value is sent over to the server:

This comment has been minimized.

Copy link
@astoff

astoff Sep 17, 2022

Contributor

I would join this line with the previous paragraph.

This comment has been minimized.

Copy link
@joaotavora

joaotavora Sep 17, 2022

Author Owner

ok


* initially, as a [`didChangeConfiguration` notification][did-change-configuration];

This comment has been minimized.

Copy link
@astoff

astoff Sep 17, 2022

Contributor

More accurately: "as a [didChangeConfiguration notification][did-change-configuration], both when the server is initialized and interactively via the eglot-signal-didChangeConfiguration command;"

This comment has been minimized.

Copy link
@joaotavora

joaotavora Sep 17, 2022

Author Owner

yeah that is more accurate, but will confuse newbies, who probably will just restart the server.

This comment has been minimized.

Copy link
@astoff

astoff Sep 17, 2022

Contributor

Fine

* as the response to [configuration request][configuration-request] from the server.

This comment has been minimized.

Copy link
@astoff

astoff Sep 17, 2022

Contributor

If we are going to make this claim, we should make sure that this feature works, which I haven't tested but I'm skeptical of.

The specification (https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#workspace_configuration) is totally vague, but if I'm guessing correctly, the section argument of the workspace.configuration argument can be a nested thingy like "pyls.plugins.jedi", and if that's correct then the current implementation is not conformant.

This comment has been minimized.

Copy link
@joaotavora

joaotavora Sep 17, 2022

Author Owner

It was added back in 2019 to do and fix things in #316, you can refer to these issues and ask the authors of this feature. I see nothing wrong with it. These are the issues that most recently touched that code.

Per #967: eglot-workspace-configuration can be a function
Fix #340: Don't choke on workspace/configuration with no scopeUri
Fix #326: support workspace/configuration

But if you can find a server to test this, and improve it, that is of course welcome.

This comment has been minimized.

Copy link
@astoff

astoff Sep 17, 2022

Contributor

I don't know of any server that needs it.


Before considering what to set the variable to, one must understand
_how_ to set it. `eglot-workspace-configuration` is a
"directory-local" variable, and setting its variable globally or
buffer-locally likely makes no sense. It should be set via
`.dir-locals.el` or equivalent mechanisms.
_how_ to set it. `eglot-workspace-configuration` is a ["directory
variable"][dir-locals-emacs-manual]. Setting it globally or
buffer-locally makes little sense. It is usually set via
`.dir-locals.el` or [special-purpose elisp
functions][dir-locals-emacs-manual].

This comment has been minimized.

Copy link
@astoff

astoff Sep 17, 2022

Contributor

Setting it globally or buffer-locally makes little sense.

I'm not sure I agree with this statement. These config options are fully namespaced, so you can safely send your gopls config to pylsp and it will just ignore it. So yes, you can set this variable globally and call it a day.

How about replacing this paragraph by

The variable `eglot-workspace-configuration` is often set as ["directory
variable"][dir-locals-emacs-manual]. See below for an example.

This comment has been minimized.

Copy link
@joaotavora

joaotavora Sep 17, 2022

Author Owner

These config options are fully namespaced, so you can safely send your gopls config to pylsp and it will just ignore it.

Yes, I mention that some lines below.

So yes, you can set this variable globally and call it a day.

You can, but then it becomes a "global" configuration which you can probably set via some other means, not a "workspace configuration". So it makes some sense, but little.


#### Format

The variable's value is an _association list_:

```
((SECTION-1 . PARAM-OBJECT-1)
...
(SECTION-N . PARAM-OBJECT-N))
```

`SECTION-N` is an Elisp keyword naming the parameter section
understood by the server. `PARAM-OBJECT-N` contains one or more
settings pertaining to that server.

The variable's value is an _association list_ of _parameter sections_
to _parameter objects_. Names and formats of section and parameter
objects are server specific.
`PARAM-OBJECT-N` is an Elisp object serialized to JSON by
[`json-serialize`][json-serialize]. The recommended format used in
this manual's examples is a [plist][plist] of keyword-value pairs,
though `json-serialize` also accepts other formats.

When experimenting with settings, one may use `M-x
eglot-show-workspace-configuration` to inspect/debug the definite JSON
value sent over to the server. This helper function works even before
actually connecting to the server.

#### Simple `eglot-workspace-configuration`

To make a particular Python project always enable Pyls's snippet
support, put a file named `.dir-locals.el` in the project's root:
support, you may put a file named `.dir-locals.el` in the project's
root:

```lisp
((python-mode
. ((eglot-workspace-configuration
. ((:pyls . (:plugins (:jedi_completion (:include_params t)))))))))
.
;; the value in the format described above starts here
((:pyls . (:plugins (:jedi_completion (:include_params t

This comment has been minimized.

Copy link
@astoff

astoff Sep 17, 2022

Contributor

You requested excruciating detail about this (#790 (comment)), but here's the summary: you can only safely use

  • plists with plain symbol keys
  • plists with keyword keys
  • alists with plain symbol keys.

This example has an alist with keyword keys. It apparently works because of the special handling of the first level of eglot-workspace-configuration, but it will for sure cause confusion.

This comment has been minimized.

Copy link
@joaotavora

joaotavora Sep 17, 2022

Author Owner

This example has an alist with keyword keys. It apparently works because of the special handling of the first level of eglot-workspace-configuration, but it will for sure cause confusion.

What confusion?The documentation wilth examples is there to explain clearly that this is an alist of objects. The key this alist is a Elisp keyword, the object can be something that json-serialize will grok. We recommend that that something is a plist. But this also works FWIW

(json-serialize '(:hey "ho" :lets ((go . 42)
                                   (fro . 84))))

So if the power-user is so inclined or -- more realistically -- has a alist generating function that whe wants to plug in, that's also fine.

If you'd like to propose code to allow the top-level structure to be a plist too, for some consistency, I think that's fine too. But it'll still look a bit off when used as a dir-local value, which uses alists.

Noone said eglot-workspace-configuration is fully serialized with json-serialize.

This comment has been minimized.

Copy link
@astoff

astoff Sep 17, 2022

Contributor

What confusion?The documentation wilth examples is there to explain clearly that this is an alist of objects.

Well, the user will not copy and paste the given configuration example; they will copy, paste and slightly modify it, while typically not knowing much about Elisp. Doesn't it make sense to get rid of all these sharp edges?

Anyone who doesn't know Lisp will wonder why a dot after :pyls but not after :plugins.

That you need a dot after eglot-workspace-configuration, which is the name of the variable you're setting, is easier to accept. You even added a comment after the dot, which is a very good idea actually.

:fuzzy t)
:pylint (:enabled :json-false)))))
;; and ends here
))))
```

This tells Emacs that any `python-mode` buffers in that directory
should have a particular value of `eglot-workspace-configuration`.

Here, the value in question associates section `:pyls` with parameters
`(:plugins (:jedi_completion (:include_params t)))`. The parameter
object is a plist converted to JSON before being sent to the server.
Here, the value in question associates a parameter section `:pyls`
with a parameter objct that is a plist of plists. It is converted to
JSON before being sent to the server:

```json
{
"pyls": {
"plugins": {
"jedi_completion": { "include_params": true, "fuzzy": true },
"pylint": { "enabled": false }
}
}
}
```

#### Multiple servers in `eglot-workspace-configuration`

Expand All @@ -182,20 +224,25 @@ a section for `go-mode`, the file's contents now become:
```lisp
((python-mode
. ((eglot-workspace-configuration
. ((:pyls . (:plugins (:jedi_completion (:include_params t))))))))
. ((:pyls . (:plugins (:jedi_completion (:include_params t
:fuzzy t)
:pylint (:enabled :json-false))))))))
(go-mode
. ((eglot-workspace-configuration
. ((:gopls . (:usePlaceholders t)))))))
```

Alternatively, as a matter of taste, you may choose this equivalent
setup, which sets the variables value in all all major modes of all
buffers of a given project.
setup. This sets the value in all major-modes inside the project: the
major-mode specification is unneeded because the LSP server will
retrieve only the parameter section it is interested in.

```lisp
((nil
. ((eglot-workspace-configuration
. ((:pyls . (:plugins (:jedi_completion (:include_params t))))
. ((:pyls . (:plugins (:jedi_completion (:include_params t
:fuzzy t)
:pylint (:enabled :json-false))))
(:gopls . (:usePlaceholders t)))))))
```

Expand All @@ -211,10 +258,10 @@ leverage per-directory local variables. Look for the functions

If you need to determine the workspace configuration base on some
dynamic context, make `eglot-workspace-configuration` a function. It
is passed the `eglot-lsp-server` instance and runs with
`default-directory` set to the root of your project. The function
should return a value of the same form as described in the previous
paragraphs.
is passed the `eglot-lsp-server` instance of the connected server (if
any) and runs with `default-directory` set to the root of your
project. The function should return a value of the same form as
described in the previous paragraphs.

## Handling quirky servers

Expand Down Expand Up @@ -626,3 +673,7 @@ for the request form, and we'll send it to you.
[copyright-assignment]: https://www.fsf.org/licensing/contributor-faq
[legally-significant]: https://www.gnu.org/prep/maintain/html_node/Legally-Significant.html#Legally-Significant
[dir-locals-emacs-manual]: https://www.gnu.org/software/emacs/manual/html_node/emacs/Directory-Variables.html
[configuration-request]: https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#workspace_configuration
[did-change-configuration]: https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#workspace_didChangeConfiguration
[json-serialize]: https://www.gnu.org/software/emacs/manual/html_node/elisp/Parsing-JSON.html
[plist]: https://www.gnu.org/software/emacs/manual/html_node/elisp/Property-Lists.html
43 changes: 25 additions & 18 deletions eglot.el
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,6 @@

;;; Code:

(require 'json)

This comment has been minimized.

Copy link
@astoff

astoff Sep 17, 2022

Contributor

Did you leave the other (require 'json) on line 77 (old 78) there on purpose? It doesn't seem necessary.

This comment has been minimized.

Copy link
@joaotavora

joaotavora Sep 17, 2022

Author Owner

yes, it can be removed.

(require 'imenu)
(require 'cl-lib)
(require 'project)
Expand Down Expand Up @@ -2204,30 +2203,43 @@ SECTION should be a keyword or a string. VALUE is a
plist or a primitive type converted to JSON.
The value of this variable can also be a unary function of a
`eglot-lsp-server' instance, the server connection requesting the
configuration. It should return an alist of the format described
above.")
single argument, which will be a connected `eglot-lsp-server'
instance. The function runs with `default-directory' set to the
root of the current project. It should return an alist of the
format described above.")

;;;###autoload
(put 'eglot-workspace-configuration 'safe-local-variable 'listp)

(defun eglot-show-configuration (server)
"Dump `eglot-workspace-configuration' as json for debugging."
(interactive (list (eglot--read-server "Server configuration"
(eglot-current-server))))
(let ((conf (eglot--workspace-configuration server)))
(with-current-buffer (get-buffer-create " *eglot configuration*")
(defun eglot-show-workspace-configuration (&optional server)
"Dump `eglot-workspace-configuration' as JSON for debugging."
(interactive (list (and (eglot-current-server)
(eglot--read-server "Server configuration"
(eglot-current-server)))))
(let ((conf (eglot--workspace-configuration-plist server)))
(with-current-buffer (get-buffer-create "*EGLOT workspace configuration*")
(erase-buffer)
(insert (jsonrpc--json-encode conf))
(json-mode)
(json-pretty-print-buffer)
(with-no-warnings
(require 'json)
(require 'json-mode)
(json-mode)
(json-pretty-print-buffer))
(pop-to-buffer (current-buffer)))))

(defun eglot--workspace-configuration (server)
(if (functionp eglot-workspace-configuration)
(funcall eglot-workspace-configuration server)
eglot-workspace-configuration))

(defun eglot--workspace-configuration-plist (server)
"Returns `eglot-workspace-configuraiton' suitable serialization."
(or (cl-loop for (section . v) in (eglot--workspace-configuration server)
collect (if (keywordp section) section
(intern (format ":%s" section)))
collect v)
eglot--{}))

(defun eglot-signal-didChangeConfiguration (server)
"Send a `:workspace/didChangeConfiguration' signal to SERVER.
When called interactively, use the currently active server"
Expand All @@ -2236,12 +2248,7 @@ When called interactively, use the currently active server"
server :workspace/didChangeConfiguration
(list
:settings
(or (cl-loop for (section . v) in (eglot--workspace-configuration server)
collect (if (keywordp section)
section
(intern (format ":%s" section)))
collect v)
eglot--{}))))
(eglot--workspace-configuration-plist server))))

(cl-defmethod eglot-handle-request
(server (_method (eql workspace/configuration)) &key items)
Expand Down

0 comments on commit 18d4627

Please sign in to comment.