search

joaotavora / eglot

A client for Language Server Protocol servers
GNU General Public License v3.0
2.25k stars 200 forks source link

Allow eglot-workspace-configuration to be a plist #1033

Closed astoff closed 2 years ago

astoff commented 2 years ago

I would also suggest changing the documentation so it refers only to plists, or only to alists. The current formulation is pretty confusing, since it mixes both things. Also, I guess we shouldn't make reference to alists with keyword or string keys, which are unusual and not accepted by json-serialize respectively.

So, in https://github.com/joaotavora/eglot#simple-eglot-workspace-configuration, either

((python-mode
  . ((eglot-workspace-configuration
      . ((pyls . (plugins . (jedi_completion . (include_params . t)))))))))

or

((python-mode
  . ((eglot-workspace-configuration
      . (:pyls (:plugins (:jedi_completion (:include_params t))))))))
fbergroth commented 2 years ago

I was recently reminded of a closed PR of mine, that does something similar; #790. It dumps the config recursively and tries to fix inconsistencies between json-encode and json-serialize. Since you also looked into this and I vaguely remember the details, do you think it covers this, and should be merged?

joaotavora commented 2 years ago

I was recently reminded of a closed PR of mine, that does something similar; https://github.com/joaotavora/eglot/pull/790.

Yes, please merge the two PRs, explain clearly:

astoff commented 2 years ago

To be honest, I'm not a fan of attempting to fix inconsistencies of the two encoders, which AFAICT are pretty mild. Specifically, the C encoder gives an error on alists and plists where the keys are strings, and it doesn't remove the leading colon if you use a keyword as a key in an alist.

I think these problems are best solved by by restricting to the usual constructs in the examples and docstrings: alists with plain symbol keys and plists with keyword keys.

If I were to attempt something smart here, I would rather allow string keys for a "flattened out" representation of deeply nested structures. So, for instance, you could write

((python-mode
  . ((eglot-workspace-configuration
      "pyls.plugins.jedi_completion.include_params" t
      "pyls.plugins.something_else" [1 2 3]))))

and this would be equivalent to saying

((python-mode
  . ((eglot-workspace-configuration
      . ((:pyls (:plugins (:jedi_completion (:include_params . t))
                           :something_else [1 2 3]))))))

My tiny merge request here is compatible with this future possiblity. How about #790?

joaotavora commented 2 years ago

Can you explain why these incompatibilities cannot or should not be addressed in jsonrpc.el?

astoff commented 2 years ago

Can you explain why these incompatibilities cannot or should not be addressed in jsonrpc.el?

This would mean that before sending every and each request, we "rewrite" the entire Lisp object which is about to be encoded and replace strings and keywords that appear as keys of an alist. This would defeat the purpose of using a fast C JSON encoder.

So I'm pretty sure there's nothing to be done in jsonrpc.el.

joaotavora commented 2 years ago

I still struggle to understand what the problem is.

Can you state again, for my benefit, exactly what the user may type today that leads to different behaviour according to the user's (unknown and underlying) use of json.el or json.c?

Thereafter state which of these two behaviours you think is the correct one.

astoff commented 2 years ago

Can you state again, for my benefit, exactly what the user may type today that leads to different behaviour according to the user's (unknown and underlying) use of json.el or json.c?

Okay, but this is more related to #790, so I've replied there.

astoff commented 2 years ago

The purpose of this PR is pretty restricted: to allow both of the following forms:

((python-mode
  . ((eglot-workspace-configuration
      . ((pyls . (plugins . (jedi_completion . (include_params . t)))))))))

or

((python-mode
  . ((eglot-workspace-configuration
      . (:pyls (:plugins (:jedi_completion (:include_params t))))))))

Currently only the first one is accepted.

joaotavora commented 2 years ago

ok thanks for the clarification.

joaotavora commented 2 years ago

The purpose of this PR is pretty restricted: to allow both of the following forms:

If possible add this text to the commit message and to the description of the PR.

Also please state clearly, for both forms, what the resulting JSON object sent to the server is in each situation (supposing it's different: if it's the same, just say so).

astoff commented 2 years ago

If you like the suggestion of https://github.com/joaotavora/eglot/pull/1033#issuecomment-1243304059, it would be better to implement it directly instead of merging this.

fbergroth commented 2 years ago

It seems this assumes configuration to be an alist? https://github.com/joaotavora/eglot/blob/06e6dd6693123a737fb883dcf277432d63d6518e/eglot.el#L2248

joaotavora commented 2 years ago

Yes, whatever the code does now is behaviour to be kept. We can of course enhance with backward-compatible things.

On Fri, Sep 16, 2022, 18:41 Fredrik Bergroth @.***> wrote:

It seems this assumes configuration to be an alist? https://github.com/joaotavora/eglot/blob/06e6dd6693123a737fb883dcf277432d63d6518e/eglot.el#L2248

— Reply to this email directly, view it on GitHub https://github.com/joaotavora/eglot/pull/1033#issuecomment-1249623092, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAC6PQ6G25DFK2YFBE4HNZTV6SWKZANCNFSM6AAAAAAQJN7GMI . You are receiving this because you commented.Message ID: @.***>

astoff commented 2 years ago

Okay, I missed the case of the workspace.configuration server -> client request. So this change, as is right now, would break this functionality. With that said:

  1. The server submits a request to the client? That's madness.
  2. The "specification" of the workspace.configuration method (https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#workspace_configuration) is totally vague. I guess the expected behavior here is "whatever VS Code does".
  3. But if I'm guessing correctly, the section argument can be a nested thingy like "pyls.plugins.jedi", and if that's correct then the current implementation is not conformant.

In my opinion Eglot should not support this method. The initializationOptions and the workspace.didChangeConfiguration method should suffice to configure the server. WDYT?

joaotavora commented 2 years ago

I'm not opposed to this PR and I think it'd be nice to allow eglot-workspace-configuration to be simply a plist, even though I think that the latest doc update makes things simple.

But it's missing this bit as pointed out earlier (https://github.com/joaotavora/eglot/pull/1033#issuecomment-1249623092).

Also I'd prefer if the change is small. If it becomes too big/complex, I don't think it's worth it.

astoff commented 2 years ago

But it's missing this bit as pointed out earlier (#1033 (comment)).

That comment points to a separate issue. Did you look at the LSP spec, where it gives "cpp.formatterOptions" as a possible value of "section" in the workspace/configuration request? Do you agree this is already not handled correctly by Eglot?

There is an interdependency between that issue and this PR. From my perspective it would be better to fix the issue after merging this PR, since only then it will be clear what are all the forms that eglot-workspace-configuration can have.

joaotavora commented 2 years ago

Do you agree this is already not handled correctly by Eglot?

I don't know. I just know that if I merge your pr and tell people they can use plists, that piece of code that was added in 2019 to fix, presumably, some real problem, will cease to work.

There is an interdependency between that issue and this PR. From my perspective it would be better to fix the issue after merging this PR, since only then it will be clear what are all the forms that eglot-workspace-configuration can have.

I'm sorry, I don't view things that way. Allowing e-w-configuration to be a plist is a nice-to-have, not a bugfix. So I don't want to break things over it.

astoff commented 2 years ago

Well, I've pushed a change to the workspace/configuration handler, but I can't test it because I have never seen this request happen. So I suspect this pr will not advance, but we'll see.

astoff commented 2 years ago

Fine, so we can close this ticket. But I doubt that https://github.com/joaotavora/eglot/commit/e4b3a5315803756f56292a04fb0b2065d6fc3e03 works. You defined a function eglot--workspace-configuration-plist that returns a plist or a hash table, and then you call plist-get on its output.

I also think you would save work if you had a function that normalizes the config to have non-keyword keys, so you can call plist-get or alist-get with a simple string= as test function. But that's just a palpite (hard to translate precisely, that word).

joaotavora commented 2 years ago

You defined a function eglot--workspace-configuration-plist that returns a plist or a hash table, and then you call plist-get on its output.

Hmm good point. I missed that, I guess I can just make it return nil

But that's just a palpite (hard to translate precisely, that word).

That would be "hunch" I think. But palpite if much better of course.