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

Use eglot--dbind for destructuring #171

Closed
wants to merge 1 commit into from
Closed

Use eglot--dbind for destructuring #171

wants to merge 1 commit into from

Conversation

mkcms
Copy link
Collaborator

@mkcms mkcms commented Nov 27, 2018

  • eglot.el (eglot--lsp-interface-alist): Add CodeAction,
    TextDocumentEdit, WorkspaceEdit.
    (eglot--apply-workspace-edit): Use eglot--dbind.
    (eglot-code-actions): Use eglot--lambda.

@mkcms mkcms added the WIP Work in progress label Nov 27, 2018
@mkcms
Copy link
Collaborator Author

mkcms commented Nov 27, 2018

@joaotavora You mentioned that eglot--lsp-interface-alist should only contain types "designated by the spec as export interface. " Is there a reason for it? What does the export keyword mean in TS?

AFAICS, most interfaces are not exported.

@mkcms mkcms changed the title WIP: Use eglot--dbind for destructuring Use eglot--dbind for destructuring Nov 28, 2018
@mkcms mkcms removed the WIP Work in progress label Nov 28, 2018
@mkcms
Copy link
Collaborator Author

mkcms commented Nov 28, 2018

I couldn't find any more uses of export interface.

* eglot.el (eglot--lsp-interface-alist): Add CodeAction,
  FileSystemWatcher, Registration, TextDocumentEdit, WorkspaceEdit.
(eglot-handle-notification): Use eglot--dbind.
(eglot--apply-workspace-edit): Use eglot--dbind and eglot--lambda.
(eglot-code-actions): Use eglot--lambda.
(eglot--register-workspace/didChangeWatchedFiles): Use eglot--lambda.
@joaotavora
Copy link
Owner

@mkcms This looks good, and by the way, is the way I would like to see it merged (adding interface specs little by little, and using eglot--dbind with no formal interface here and there).

The only thing that I am considering reviewing is the eglot--dbind interface. I think we don't need the &key at all. And I also think that the interface spec could be inside the formal arg list, but in a sublist in the first position. So

(eglot--dbind (foo bar baz) obj ...)

Would do what (eglot--dbind nil (&key foo bar baz) obj...) does now. And

(eglot--dbind ((TheFoo) foo bar baz) obj ...)

Would do what (eglot--dbind TheFoo (&key foo bar baz) obj ...) does now.

What do you think?

Regarding export interface, I'll have a second look. I looked at a few and found that, so assumed most important external interfaces would be like that... But it's not crucial, I think. I don't know exactly what format/informal language the spec is built upon, but it smells vaguely of C# or Java or something.

@mkcms
Copy link
Collaborator Author

mkcms commented Nov 28, 2018

What do you think?

Looks good, I also wanted to propose getting rid of &key because it's redundant and because when implementing the pcase macro, it was complicating things.

@joaotavora
Copy link
Owner

Regarding the pcase implementation, I wanted to say that it is only that, an implementation. I think I shall move forward with a pcase-less eglot--dcase. Then, when we have the pcase implementation, we can probably try fancier things like.

(eglot--dbind ((or FooObject NearFooObject) foo bar baz) obj ...)

Or something like that. Or even use pcase directly.

@mkcms
Copy link
Collaborator Author

mkcms commented Nov 28, 2018

Regarding the pcase implementation, I wanted to say that it is only that, an implementation. I think I shall move forward with a pcase-less eglot--dcase. Then, when we have the pcase implementation

Sorry, I don't understand, should I ditch the pcase PR?

@joaotavora
Copy link
Owner

Sorry, I don't understand, should I ditch the pcase PR?

No. It's just that it's not really ready yet. (for example, it uses eval, which is generally sign of problems).

So I'm going to do a simpler eglot--dcase for now. And then we can upgrade to pcase which could (probably should) make eglot--dcase obsolete.

@mkcms
Copy link
Collaborator Author

mkcms commented Nov 28, 2018

No. It's just that it's not really ready yet. (for example, it uses eval, which is generally sign of problems).

OK. Do you have a suggestion what I should use instead? I couldn't find anything in the Emacs sources for dynamic macro expansion.

joaotavora added a commit that referenced this pull request Nov 29, 2018
* eglot.el (eglot--dcase): New macro.
@joaotavora
Copy link
Owner

OK. Do you have a suggestion what I should use instead? I couldn't find anything in the Emacs sources for dynamic macro expansion.

Macros are a black art (I'm not particularly good at them). But once you understand the macroexpander you should be able to generate code at macroexpansion-time without ever using eval. Here's what helps me think. Everytime the macroexpander looks at a top level form of a macro that it knows, it writes in the macroexpansion. That macroexpansion can use variables, functions, other macros, that are available at macroexpansion time (which is quite different from run-time, or even before the file is loaded).

I'm sorry if I can't be more help. If you write Stefan Monnier and explain the problem (or write to emacs-help with CC: to him), I'm sure he will help you with your problem.

Anyway, as promised, I pushed a preliminary version (possibly buggy) of an eglot--dcase macro. We should make that our interface, and vary the implementation to pcase once it's ready.

joaotavora added a commit that referenced this pull request Dec 1, 2018
* eglot.el (eglot--dcase): New macro.

* eglot-tests.el (eglot-dcase-with-interface)
(eglot-dcase-no-interface): New tests.
@joaotavora
Copy link
Owner

Merged this after updating to the new eglot--dbind protocol.

@joaotavora joaotavora closed this Dec 1, 2018
@mkcms
Copy link
Collaborator Author

mkcms commented Dec 2, 2018

@joaotavora So what about interfaces which are not export interface?

@joaotavora
Copy link
Owner

@joaotavora So what about interfaces which are not export interface?

They could be added to eglot--lsp-interface-alist as well, I guess. I haven't yet understood what the difference is between an export an non-export interface.

bhankas pushed a commit to bhankas/emacs that referenced this pull request Sep 18, 2022
* eglot.el (eglot--dcase): New macro.

* eglot-tests.el (eglot-dcase-with-interface)
(eglot-dcase-no-interface): New tests.
bhankas pushed a commit to bhankas/emacs that referenced this pull request Sep 19, 2022
* eglot.el (eglot--dcase): New macro.

* eglot-tests.el (eglot-dcase-with-interface)
(eglot-dcase-no-interface): New tests.
bhankas pushed a commit to bhankas/emacs that referenced this pull request Sep 19, 2022
* eglot.el (eglot--dcase): New macro.

* eglot-tests.el (eglot-dcase-with-interface)
(eglot-dcase-no-interface): New tests.

#171: joaotavora/eglot#171
,#156: joaotavora/eglot#156
jollaitbot pushed a commit to sailfishos-mirror/emacs that referenced this pull request Oct 12, 2022
* eglot.el (eglot--dcase): New macro.

* eglot-tests.el (eglot-dcase-with-interface)
(eglot-dcase-no-interface): New tests.

GitHub-reference: per joaotavora/eglot#171
GitHub-reference: per joaotavora/eglot#156
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants