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

Populate completions with metadata #3226

Merged
merged 11 commits into from
Jul 28, 2022
Merged

Conversation

toniz4
Copy link
Contributor

@toniz4 toniz4 commented Jul 25, 2022

This makes possible to override the cider completion style in completion-category-defaults or completion-category-overrides.

This will simplify the workarounds needed to make cider work with orderless set as the "global" completion style. like seen in #3019 (comment). With this pull request, the following will work.

(setq completion-category-defaults '((cider (styles basic)))

The code is basicaly the same as the function returned by completion-table-dynamic, but simplified since we don't need to check switch-buffer, since is always nil in this context. And when (eq action 'metadata), return the metadata.

completion-table-dynamic, in minibuffer.el

(defun completion-table-dynamic (fun &optional switch-buffer)
  (lambda (string pred action)
    (if (or (eq (car-safe action) 'boundaries) (eq action 'metadata))
        ;; `fun' is not supposed to return another function but a plain old
        ;; completion table, whose boundaries are always trivial.
        nil
      (with-current-buffer (if (not switch-buffer) (current-buffer)
                             (let ((win (minibuffer-selected-window)))
                               (if (window-live-p win) (window-buffer win)
                                 (current-buffer))))
        (complete-with-action action (funcall fun string) string pred)))))

The implementation in the pull request

(lambda (string pred action)
              (cond ((eq action 'metadata) `(metadata (category . cider)))
                    ((eq (car-safe action) 'boundaries) nil)
                    (t (with-current-buffer (current-buffer)
                         (complete-with-action action
                                               (cider-complete string) string pred)))))

I've been using this implementation for 2 days with advice-add, experienced no problems.

Let me know if its needed to add any documentation around this .


Before submitting the PR make sure the following things have been done (and denote this
by checking the relevant checkboxes):

  • The commits are consistent with our contribution guidelines
  • You've added tests (if possible) to cover your change(s)
  • All tests are passing (eldev test)
  • All code passes the linter (eldev lint) which is based on elisp-lint and includes
  • You've updated the changelog (if adding/changing user-visible functionality)
  • You've updated the user manual (if adding/changing user-visible functionality)

Thanks!

If you're just starting out to hack on CIDER you might find this section of its
manual
extremely useful.

@bbatsov
Copy link
Member

bbatsov commented Jul 25, 2022

@dgutov Can you take a look at the proposed changes as an expert in the API in question?

cider-completion.el Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
@dgutov
Copy link
Contributor

dgutov commented Jul 25, 2022

The meta -> (category . cider) part seems sound.

Though I don't know if it's a good idea to create a category per completion source. Perhaps @minad or @monnier have some opinion.

@monnier
Copy link

monnier commented Jul 25, 2022 via email

@bbatsov
Copy link
Member

bbatsov commented Jul 25, 2022

So can someone give me some examples of categories? I get this should be the implementation (e.g. CIDER), so what is it supposed to be?

@toniz4
Copy link
Contributor Author

toniz4 commented Jul 25, 2022

So can someone give me some examples of categories? I get this should be the implementation (e.g. CIDER), so what is it supposed to be?

you can see that with describe-variable completion-category-defaults, by default it comes pre-populated with some categories.

I agree with @monnier, but this is the only way that i know to get a finer control of the completion style, i.e. only apply a specific style for completions comming from cider.

Eglot and lsp-mode follows the same idea:

https://github.com/emacs-lsp/lsp-mode/blob/b2dc84128fc6b2cd0460569bae6d0451856d2e0b/lsp-completion.el#L514

https://github.com/joaotavora/eglot/blob/6cc6fcc0fabb51671980e52110ad74f2e691e387/eglot.el#L2613

Eglot sets the category as eglot, and lsp-mode sets it to lsp-capf

cider-completion.el Outdated Show resolved Hide resolved
cider-completion.el Outdated Show resolved Hide resolved
@bbatsov
Copy link
Member

bbatsov commented Jul 25, 2022

Fair enough. It'd be good if you also added something in the completion docs for (setq completion-category-defaults '((cider (styles basic))).

@toniz4 toniz4 changed the title Populate completions with metadata and make completions non exclusive Populate completions with metadata Jul 25, 2022
@minad
Copy link

minad commented Jul 25, 2022

@toniz4 Since you touch the completion code here, it would be great if #3006 could be addressed too, such that the default cider completion style becomes more compliant.

@toniz4
Copy link
Contributor Author

toniz4 commented Jul 25, 2022

@toniz4 Since you touch the completion code here, it would be great if #3006 could be addressed too, such that the default cider completion style becomes more compliant.

Yes, I opened this PR following that rabbit role, but I think I will try to adress that problem in another PR. I will have to understand how the cider completion works in a deeper level.

The interesting thing that I found is that the builtin emacs funcions for building capf's are kinda non-complient, like (complete-with-action) or (completion-table-dynamic). Or i'm undersanding stuff wrong, documentation is rare about this stuff.

@minad Where do you find good documentation about capf's? Or the emacs manuals should be enought?

@minad
Copy link

minad commented Jul 25, 2022

The interesting thing that I found is that the builtin emacs funcions for building capf's are kinda non-complient, like (complete-with-action) or (completion-table-dynamic). Or i'm undersanding stuff wrong, documentation is rare about this stuff.

The limitation is that completion-table-dynamic requires you to use the basic, emacs21 or emacs22 completion styles such that the input string is passed to the completion table for dynamic candidate computation.

Where do you find good documentation about capf's? Or the emacs manuals should be enought?

For Capfs the Emacs manual is not really a sufficient resource, but you can grep for example Capfs. My Cape package also comes with a bunch of simple Capfs which you could use as reference.


I just looked at #3006 again. My recommendation is to remove the cider completion style completely:

cider/cider-completion.el

Lines 281 to 295 in b47fe53

;; Fuzzy completion for company-mode
(defun cider-company-unfiltered-candidates (string &rest _)
"Return CIDER completion candidates for STRING as is, unfiltered."
(cider-complete string))
(add-to-list 'completion-styles-alist
'(cider
cider-company-unfiltered-candidates
cider-company-unfiltered-candidates
"CIDER backend-driven completion style."))
(defun cider-company-enable-fuzzy-completion ()
"Enable backend-driven fuzzy completion in the current buffer."
(setq-local completion-styles '(cider)))

Instead configure the basic completion style for the cider category you introduced above.

@monnier
Copy link

monnier commented Jul 25, 2022 via email

@toniz4
Copy link
Contributor Author

toniz4 commented Jul 26, 2022

I agree with @monnier, but this is the only way that i know to get a finer control of the completion style, i.e. only apply a specific style for completions comming from cider.
I'd need to know why you want the completion style to be different when you use CIDER to be able to start thinking a bout what's a better answer. Is CIDER used only for one particular kind of completion (e.g. is it only used to complete function names, or also file names, module names, ...)?

I use orderless as the completion style globaly, and I would like to keep it that way for most of the capf's, but some capf's don't play nicely with that, as #3006 and the last comment made by minad shows. And it's nice to have the option to set a different completion style in this category, orderless gives you the option of making custom completion engines.

I could set completion-styles to basic localy when using cider, but every other completion will use basic too, if not overwriten in completion-category-defaults or completion-category-overrides, so it would fix one part and break the rest (just in the buffer with cider running, but this is inconsistent for me).

@minad
Copy link

minad commented Jul 26, 2022

Using a completion category here is valid. But you could also set globally:

(setq completion-styles '(orderless basic)

This is the recommended configuration from the Orderless configuration and will make sure that completion-table-dynamic works properly.

@bbatsov bbatsov merged commit f422665 into clojure-emacs:master Jul 28, 2022
@monnier
Copy link

monnier commented Oct 11, 2022 via email

vemv added a commit that referenced this pull request Oct 8, 2023
This possibility was enabled by #3226

It can be seen in use in SystemCrafters/crafted-emacs#241 or https://clojurians.slack.com/archives/C0617A8PQ/p1696684010392119?thread_ts=1696618691.247989&cid=C0617A8PQ|

It provides a better default experience for those using orderless or other newer styles.
vemv added a commit that referenced this pull request Oct 8, 2023
This possibility was enabled by #3226

It can be seen in use in SystemCrafters/crafted-emacs#241 or https://clojurians.slack.com/archives/C0617A8PQ/p1696684010392119?thread_ts=1696618691.247989&cid=C0617A8PQ|

It provides a better default experience for those using orderless or other newer styles.
vemv added a commit that referenced this pull request Oct 8, 2023
This possibility was enabled by #3226

It can be seen in use in SystemCrafters/crafted-emacs#241 or https://clojurians.slack.com/archives/C0617A8PQ/p1696684010392119?thread_ts=1696618691.247989&cid=C0617A8PQ|

It provides a better default experience for those using orderless or other newer styles.
bbatsov pushed a commit that referenced this pull request Oct 9, 2023
This possibility was enabled by #3226

It can be seen in use in SystemCrafters/crafted-emacs#241 or https://clojurians.slack.com/archives/C0617A8PQ/p1696684010392119?thread_ts=1696618691.247989&cid=C0617A8PQ|

It provides a better default experience for those using orderless or other newer styles.
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.

5 participants