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

All completions #733

Closed
wants to merge 2 commits into from
Closed

Conversation

jdtsmith
Copy link

@jdtsmith jdtsmith commented Sep 4, 2021

This uses all-completions to enable searching of candidates with completion-regexp-list, which CAPF collection functions are required to do (from elisp/basic completion info):

In addition, to be acceptable, a completion must also match all the regular expressions in ‘completion-regexp-list’. (Unless COLLECTION is a function, in which case that function has to handle ‘completion-regexp-list’ itself.)

It also ensures any predicate passed to the collection function is forwarded to try/test-completion.

@joaotavora
Copy link
Owner

joaotavora commented Sep 4, 2021

It also ensures any predicate passed to the collection function is forwarded to try/test-completion.

Can you demonstrate the practical purpose of this? I.e. can you show what undesired thing happened before 16446e5 which doesn't happen after 16446e5?

@joaotavora
Copy link
Owner

In ebda0b3, you haven't explained why you changed the body of the lambda passed to all-completions. Please do that.

Meanwhile, for organizational reasons, I will close this PR. We can continue in the issue where we were discussing things before.

@joaotavora joaotavora closed this Sep 4, 2021
@jdtsmith
Copy link
Author

jdtsmith commented Sep 5, 2021

It also ensures any predicate passed to the collection function is forwarded to try/test-completion.

Can you demonstrate the practical purpose of this? I.e. can you show what undesired thing happened before 16446e5 which doesn't happen after 16446e5?

The practical purpose is following the programmed completion specification to avoid a subtly broken CAPF table. If any code inserts a predicate anywhere, including these pred pass-throughs will ensure they work correctly. Not including them, or including them only for all-completions as has been the case, will result in inconsistent completion table behavior.

Specific example: suppose a simple completion predicate is inserted by the user which filters out the word "beans". Your all-completions may receive '("franks" "beans" "slaw" "corn") and filter it to '("franks" "slaw" "corn"). Yet try-completion with your current table will report "beans" is a valid completion.

If you are still concerned about passing pred, see the code for complete-with-action, which is where most dynamic programmed completion functions eventually land:

(t
    (funcall
     (cond
      ((null action) 'try-completion)
      ((eq action t) 'all-completions)
      (t 'test-completion))
     string collection predicate))

@jdtsmith
Copy link
Author

jdtsmith commented Sep 5, 2021

In ebda0b3, you haven't explained why you changed the body of the lambda passed to all-completions. Please do that.

Meanwhile, for organizational reasons, I will close this PR. We can continue in the issue where we were discussing things before.

See the discussion. I have reverted to your lambda in my branch which this PR tracks/tracked.

@joaotavora
Copy link
Owner

@jdtsmith I've pushed the main "remove-if-not -> all-completions" commit. If you can provide a real example of predicate "subtle broken" things like you did for the completion-regexp-alist I'll consider pushing that. But it should to be actual things that a user does, not hypothetical suppositions of breakage ((i.e. how does a user "insert a simple completion predicate"?). The change may look good but without a proven practical effect, I just won't do it: this function is way too hairy to touch just because it seems correct.

@jdtsmith
Copy link
Author

jdtsmith commented Sep 5, 2021

OK, thanks. I must admit that I find your position on the predicate hard to comprehend. I presume you read the part in the link to programmed completion info that requires compliant CAPF dynamic functions to use the predicate to filter their results:

The function should call the predicate for each possible match, and ignore the match if the predicate returns nil.

That is accomplished in eglot's case by simply passing the predicate on to the final fixed-table calls of try/test/all-completions.

A real example from consult: it can map completion-at-point into completing-read in the minibuffer, and it uses the :predicate property (which will eventually get passed as the 3rd argument to your CAPF collection function) to narrow by category. If I wanted to write a consult-eglot front end which allowed me to filter eglot completion results in the minibuffer by the kind of completion (variable, function, etc.) this would not work reliably.

Does the absence of predicate compliance break any user's eglot setup? I don't know. Does remaining out of compliance with the CAPF spec make that a possibility? Absolutely.

Obviously your call but that's my final $2e-2.

Thanks for eglot, working well here.

@joaotavora
Copy link
Owner

joaotavora commented Sep 5, 2021

I think my position is pretty easy to comprehend. When you can show an actual benefic effect deriving from a code change that happens in what we can call reality, I'll do that change. Every change in Eglot happened from this principle, not because something sounded reasonable or good. The reality of adapting LSP to Emacs doesn't always marry perfectly with Emacs's ideal APIs. Emacs' idea of "prefix" is completely wasted here, among many other other LSP/Emacs quirks. Furthermore this is one of the hairiest and most often changed functions (you can see this from its history), that has to provide support to a zillion servers via a rather flimsy and somewhat ad-hoc protocol.

If I wanted to write a consult-eglot front end which allowed me to filter eglot completion results in the minibuffer by the kind of completion (variable, function, etc.) this would not work reliably.

OK then, once you actually write this thing and it turns out you really needed that bit of code, we can revisit this, no problem.

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