Skip to content

Introduce cider-enable-flex-completion #3509

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

Merged
merged 6 commits into from
Oct 12, 2023
Merged

Introduce cider-enable-flex-completion #3509

merged 6 commits into from
Oct 12, 2023

Conversation

vemv
Copy link
Member

@vemv vemv commented Oct 10, 2023

Replaces cider-company-enable-fuzzy-completion, which is now deprecated.

This way, the flex completion style becomes opt-in (7e68df had inadvertently enabled it unconditionally).

I have QAd this and it works as intended:

  • iteatn<TAB> will complete nothing by default
  • after M-x cider-enable-flex-completion, iteatn<TAB> will complete iteration.

I also added comments so that we better understand things in a future.

Cheers - V

@vemv vemv requested a review from bbatsov October 10, 2023 12:02
(setq-local completion-styles '(cider)))

(make-obsolete 'cider-company-enable-fuzzy-completion 'cider-company-enable-fuzzy-completion-globally "1.8.0")

(defun cider-company-enable-fuzzy-completion-globally ()
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The old cider-company-enable-fuzzy-completion worked on a per-buffer fashion, which isn't necessarily the best UX.

cider-company-enable-fuzzy-completion-globally can afford to work globally instead, because completion-category-overrides works on our own cider category, which allows us to operate "globally within a namespace" i.e. safely.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I get this, although for the end users it'd be more or less the same (as the hook would be triggered in any cider-mode buffer), so I'm big concerned the name might be confusing. That was part of the reason I suggested the use of flex instead of fuzzy. I'm also thinking that is not company specific, so we should probably drop company from the name.

@bbatsov
Copy link
Member

bbatsov commented Oct 12, 2023

As mentioned on Slack my main concern is whether this should be used via a hook, given that it seems to be modifying some global settings and functions triggered by hooks usually are buffer-local.

I'm guessing this can just be some function that users can trigger manually in their setups. Perhaps something like cider-company-enable-flex-completion would be a better name, as I think the term "globally" is confusing here. Probably there should also be a note that the old setup ins the only option for people on Emacs 26.

(interactive)
(when (< emacs-major-version 27)
(user-error "`cider-company-enable-fuzzy-completion-globally' requires Emacs 27 or later"))
(let ((found-styles (when-let ((cider (assq 'cider completion-category-overrides)))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This you base this code on something from another mode? I'm a bit surprised the that setting those completion-category-overrides has so many steps, but I'm also not very familiar with the modern completion engine in Emacs.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is simply for updating a nested structure like ((foo (styles bar) (cycle t)) (cider (styles basic flex))) in a way that preserves other entries.

Sadly we don't have something as handy as update-in, it seems!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see.

;; note that there's already a completion category named `cider' (grep for `(metadata (category . cider))` in this file),
;; which can be confusing given the identical name.
;; The `cider' completion style should be removed because the `flex' style is essentially equivalent.
;; (`flex' was introduced 3 years later, to be fair)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's best to use specific years (e.g. 2020 or even better - Emacs 27) here, as comments become outdated at some point.

Now, `company-mode` will accept certain fuzziness when matching
candidates against the prefix. For example, typing `mi` will show you
`map-indexed` as one of the possible completion candidates and `cji`
will complete to `clojure.java.io`. Different completion examples are
shown
https://github.com/alexander-yakushev/compliment/wiki/Examples[here].

NOTE: `cider-company-enable-fuzzy-completion` (now depreecated) should be used for Emacs < 27.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

deprecated

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, you already had that note that I suggested elsewhere. :-)

@bbatsov
Copy link
Member

bbatsov commented Oct 12, 2023

Perhaps it would be a bad idea to check what other modes that support flex are doing in terms of init code.

vemv added 3 commits October 12, 2023 17:31
Replaces `cider-company-enable-fuzzy-completion`, which is now deprecated.

This way, the `flex` completion style becomes opt-in (7e68df had inadverntently enabled it unconditionally).
Copy link
Member Author

@vemv vemv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps it would be a bad idea to check what other modes that support flex are doing in terms of init code.

The best thing we could do is to implement completion comprehensively such that all styles will work OOTB. It just happens to not be so easy.

That way we could remove the completion-category-overrides customizations, I'd consider it a temporary measure for having control of what really intend to be functional as of today.

In a future, there would be no cider-company-enable-fuzzy-completion-globally or any similar function - users would just add flex in a standard fashion.

(interactive)
(when (< emacs-major-version 27)
(user-error "`cider-company-enable-fuzzy-completion-globally' requires Emacs 27 or later"))
(let ((found-styles (when-let ((cider (assq 'cider completion-category-overrides)))
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is simply for updating a nested structure like ((foo (styles bar) (cycle t)) (cider (styles basic flex))) in a way that preserves other entries.

Sadly we don't have something as handy as update-in, it seems!

@@ -140,17 +141,21 @@ CIDER-specific "fuzzy completion" by adding:

[source,lisp]
----
(add-hook 'cider-repl-mode-hook #'cider-company-enable-fuzzy-completion)
(add-hook 'cider-mode-hook #'cider-company-enable-fuzzy-completion)
(add-hook 'cider-repl-mode-hook #'cider-company-enable-fuzzy-completion-globally)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this needs to be a hook, given what the function does - it should just be something the users add to their CIDER config.

@vemv vemv changed the title Introduce cider-company-enable-fuzzy-completion-globally Introduce cider-enable-flex-completion Oct 12, 2023
@vemv
Copy link
Member Author

vemv commented Oct 12, 2023

Ready again!

(defun cider-enable-flex-completion ()
"Enables `flex' (fuzzy) completion for CIDER in all buffers.

Only affects the `cider' comlpetion category.`"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

completion

@bbatsov
Copy link
Member

bbatsov commented Oct 12, 2023

Other than the small typo I mentioned - looks good to go.

@vemv vemv merged commit 36737a9 into master Oct 12, 2023
@vemv vemv deleted the 3019 branch October 12, 2023 20:16
@vemv
Copy link
Member Author

vemv commented Oct 12, 2023

Thanks!

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