-
-
Notifications
You must be signed in to change notification settings - Fork 647
[completion] Enable cider-completion-style by default #3797
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
Conversation
cfe9735
to
bcaceda
Compare
cider-completion.el
Outdated
|
||
;; Enable cider-completion-style by default. Users can opt out of this by | ||
;; running (cider-enable-cider-completion-style -1) | ||
(cider-enable-cider-completion-style 1) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should do this in the minor-mode, as it's not very nice to run code at the top-level in general.
|
||
This style supports non-prefix completion candidates returned by the | ||
completion backend. Only affects the `cider' completion category." | ||
completion backend. Only affects the `cider' completion category. If ARG | ||
is `1' or nil, enables the custom completion style; if `-1', disables it." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might be more idiomatic for anything non-nil to enable it and for nil
to disable it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The problem is that people currently have this function in their configs, and it now accepts zero arguments. To reuse the same function, we need to make ARG optional, and it means it arrives as nil when people want to enable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bbatsov So is this OK to leave as is or is there a better solution?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I guess it is.
No objections from me! |
bcaceda
to
47c7487
Compare
47c7487
to
9768973
Compare
I'm feeling lucky about enabling our custom
cider-completion-style
by default. We had some troubles with it in the past – it worked not according to how completion spec expected it, but now these issues are fixed as far as I can tell. The other reason why it took long to fix it is that this was optional and I don't think many users enabled it, so we didn't receive enough data about the bug we had.Anyway, I've been using this since forever and I'm confident now that this is a net positive feature to have on for everybody. I've modified
cider-enable-cider-completion-style
to acceptARG = -1
to have a way for users to disable this if they don't want it (in the same way as they now enable it).