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

Add sensible category overrides for cider #3504

Merged
merged 2 commits into from
Oct 9, 2023

Conversation

vemv
Copy link
Member

@vemv vemv commented 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.

(Ideally we'd just do whatever is needed to implement orderless etc, but we have plenty of stuff on the table. Anyone can pick up #3019 (comment))

Cheers - V

@vemv vemv requested a review from bbatsov October 8, 2023 12:19
;; Currently CIDER completions only work for `basic`, and not `flex` `initials` `partial-completion`, `orderless`, etc.
;; So we ensure that those other styles aren't used with CIDER, otherwise one would see bad or no completions at all.
;; This `add-to-list` call can be removed once we implement the other completion styles.
(add-to-list 'completion-category-overrides '(cider (styles basic)))
Copy link
Member

Choose a reason for hiding this comment

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

Something like this should not be at the top-level IMO, but rather at the mode init. (and should be cleaned when CIDER is disabled). It's not nice for packages to pollute the Emacs global config at load time.

Copy link
Member Author

Choose a reason for hiding this comment

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

I understand the concern, however we already perform add-to-list 'completion-styles-alist '(cider above these LOCs.

If we define cider globally, it seems only fair to also declare globally the extent of usefulness of cider.

The impact of the customization is limited to our own completion style, so the alternative would not add much IMO and would be harder to keep track of (vs. having everything in contiguous LOCs)

Copy link
Member

Choose a reason for hiding this comment

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

Hmm, it seem to me that the preceding expression should have been in the mode setup as well. It's not a big deal in general - obvious the code will work either way. It's more about mode hygiene and not making permanent modifications to global settings.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'd be concerned about having too many moving pieces to achieve an otherwise harmless thing.

Since we only affect cider aspects here, all modifications are 'namespaced' and aren't too different from e.g. defuns (which also techically alter the global environment).

Copy link
Member

Choose a reason for hiding this comment

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

As I said - it's just a matter of modes keeping tidy after themselves. Obviously this completion backend is mostly useless without cider-mode being active. But there's not much point in discussing this - it will work as it is and it's not a big deal.

@bbatsov
Copy link
Member

bbatsov commented Oct 8, 2023

(Ideally we'd just do whatever is needed to implement orderless etc, but we have plenty of stuff on the table. Anyone can pick up #3019 (comment))

Yeah, I never got to investigating what's actually needed for those. I'm OK with those changes provided they don't break the fuzzy completion (which Emacs calls "flex") in company-mode.

@bbatsov
Copy link
Member

bbatsov commented Oct 8, 2023

We should also document this somewhere (what's supported and what's not - seems we don't support much today).

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 vemv force-pushed the completion-category-overrides branch from 295b84e to 502c78c Compare October 8, 2023 20:11
@vemv
Copy link
Member Author

vemv commented Oct 8, 2023

flex had to be declared for the Company fuzzy matching to work (I tested it out).

I added doc as well.

@bbatsov bbatsov merged commit 7e68df2 into master Oct 9, 2023
26 checks passed
@bbatsov bbatsov deleted the completion-category-overrides branch October 9, 2023 08:04
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