-
-
Notifications
You must be signed in to change notification settings - Fork 643
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 customizable variables for cheatsheet #3690
Conversation
@bbatsov Can I be added to the |
cider-cheatsheet.el
Outdated
|
||
(defcustom cider-cheatsheet-auto-select-buffer t | ||
"Whether to auto-select the cheatsheet popup buffer." | ||
:type 'boolean) |
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.
You should also add :package-version
properties to all defcustoms
(saying 1.15.0
as the version).
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.
Done.
cider-cheatsheet.el
Outdated
(const :tag "ClojureDocs" cider-clojuredocs-lookup) | ||
function)) | ||
|
||
(defcustom cider-cheatsheet-select-path nil |
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.
Probably it'd better for this to be an option taking some symbol (the name of the type of behavior to use) instead of a boolean - it would make the code more flexible if we decide to add different options down the road.
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 that it's a good idea to create a variable like cider-cheatsheet-select-type
or cider-cheatsheet-select-display
, which accepts symbols like multi
(default) and path
. However, one small consequence of this approach is that we won't be able to use cider-cheatsheet-select
with a prefix argument anymore as it doesn’t make sense to negate the value of this variable as it did with cider-cheatsheet-select-path
, which accepted a boolean. Will this be okay?
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.
Actually, we can do something similar. We just need to check the exact prefix argument value and use the value of cider-cheatsheet-select-type
accordingly, as it is done in cider-jack-in-universal
, right?
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.
That's one option, another would be to just drop the prefix argument and rely only on the defcustom.
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 it will be better to drop the prefix argument as most users will probably always use one way or the other.
cider-cheatsheet.el
Outdated
"Whether to auto-select the cheatsheet popup buffer." | ||
:type 'boolean) | ||
|
||
(defcustom cider-cheatsheet-doc-function #'cider-doc-lookup |
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.
Maybe use a more generic name like cider-cheatsheet-(default)-action-function
? And obviously in the buffer view we can also have more actions bound to different keybindings (the default could be the one triggered by RET)
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'm not exactly sure what you mean. Maybe you can point me to some place where such logic is already implemented, so I can understand it better? I think some examples of values for such a variable will also be helpful.
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.
If I understand correctly, you're suggesting renaming it to a more generic name because it's not guaranteed that by selecting a var, the user wants to show documentation. Did I understand that correctly?
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, exactly. You can check the namespace navigator or apropos where you can trigger different actions in the buffer displaying the results (e.g. you can show the docs or go to the definition for some var), and there's some default action triggered by RET
. I'm not sure if that makes much sense here, but it's another case where we are dealing with var names, so we might aim for some consistency.
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 agree, and it actually looks more generic, which is a good thing.
cider-cheatsheet.el
Outdated
such as '/' and '&'." | ||
:type 'string) | ||
|
||
(defcustom cider-cheatsheet-indentation-function |
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'm not quite certain that many people will want to modify this. Do you have any particular use-case in mind?
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 thought that, since it is related to UI, some users might want to modify how it looks. However, I agree that most users won't do this, and it is actually not that safe to modify that variable, as it can break cider-cheatsheet-select
. So, I think it would be a good idea to move it to a constant.
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.
Oh, I assumed you meant cider-cheatsheet-select-path-separator
. If it is cider-cheatsheet-indentation-function
, then some people prefer 2 spaces, others 4 spaces, tabs, or other indication of indentation. This use-case is what I had in mind when adding this as a customizable variable.
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.
Seems like premature optimization to me. I usually wait for the users to request some config options if I have doubts that there would be demand for them.
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 that's a good approach, so for now, they will be removed.
Sure. |
3dbd140
to
162ff4b
Compare
Thank you! I'll update the package commentary in a separate PR after this one is finished. |
162ff4b
to
d43d89e
Compare
I've made all the changes as discussed in the review:
|
cider-cheatsheet.el
Outdated
If `path`, then represent each completion candidate as a full path to a var." | ||
:type '(choice (const multi) | ||
(const path)) | ||
:package-version '(cider . "0.15.0")) |
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.
1.15.0 :-)
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.
My bad, fixed!
cider-cheatsheet.el
Outdated
function) | ||
:package-version '(cider . "0.15.0")) | ||
|
||
(defcustom cider-cheatsheet-select-type 'multi |
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.
Let me think a bit about the names multi
and path
, because I'm not sure those are optimal here.
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.
Sure. I think so as well, but it was tough to come up with something better.
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.
Some alternatives I came up with are multi-step
and full-path
, or nested
and flat
.
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.
Well, come to think of it, maybe just having a prefix argument for cider-cheatsheet-select
will be enough for now.
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.
Fair enough. :-)
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.
Okay, I've pushed the change and updated the docs in #3695.
28ef58e
to
1455194
Compare
1455194
to
aec4560
Compare
Thanks! |
Add multiple customizable variables to control different aspects of
cider-cheatsheet
andcider-cheatsheet-select
behavior.