Add selectrum-move-default-candidate #333
Conversation
clemera
changed the title
|
@clemera I am also good with the way things are now, since I have all the selectrum stuff decoupled from the rest via the advice. So metadata would only help me when I set the variables in the consult--read directly. Therefore I am also good with the current way of doing things. If you merge this, I will adapt consult accordingly, such that it uses the public variable. |
selectrum.el
Outdated
| @@ -1606,7 +1606,7 @@ Otherwise, just eval BODY." | |||
| (cl-defun selectrum-read | |||
| (prompt candidates &rest args &key | |||
| default-candidate initial-input require-match | |||
| history no-move-default-candidate | |||
There was a problem hiding this comment.
Why are you modifying the API? Is this not yet part of the released API or are you gradually downstripping the API (I am all for that!)
There was a problem hiding this comment.
Well you are too fast as always :D
There was a problem hiding this comment.
Sorry, I should probably not watch this repository ;) Maybe I switch my watching mode to only watch branches/issues I am involved in. But this wouldn't have prevented me from looking here. Actually I just wanted to see if you already merged this, such that I should adapt Consult.
There was a problem hiding this comment.
No problem, I'm not used to someone watching so sometimes I'm pushing intermediate states but I'm starting to adapt ;)
There was a problem hiding this comment.
So I will ignore the option for now, so this won't break any code but they would have to update it to use the variable. We could also wait and then do all breaking changes at once, would you prefer that?
There was a problem hiding this comment.
I already answered independently, yes I would keep everything as is. And then just deprecate selectrum-read at once if this is what you are up to. I usually try to keep changes somehow separated instead of merging multiple things, but maybe my chaotic development style on the consult repo with many small commits suggests otherwise. Let's say I at least try to do it when implementing major features or large refactorings.
There was a problem hiding this comment.
I will continue in this PR to make selectrum-read private then. My hope is that not to many use it, the longer the wait the more could be affected anyway.
There was a problem hiding this comment.
Nah I will make it private in another PR and then continue with this one.
There was a problem hiding this comment.
@minad I'm also glad to get some feedback, especially when as valuable as yours. I just have to change my habits a bit, I'm very used to push things once in a while, and I'm trying things along the way. When I add a changelog that tells usually that things are in a state how I would like to submit them.
clemera
force-pushed
the
make-selectrum-move-default-candidate-public
branch
from
4f72653
to
d2e9663
Compare
| selected initially. This is handy for `switch-to-buffer' and | ||
| friends, for which getting the candidate list out of order at all | ||
| is very confusing. | ||
| NO-MOVE-DEFAULT-CANDIDATE is ignored and only kept for backward |
There was a problem hiding this comment.
I still wonder about this change. Why not just keep the current behavior?
My suggestion would be the following:
- Expose all the functionality to want to make customizable via variables. Variables are your preferred way and I think now that it is okay! At least when something like Rework state management #322 is under consideration.
- Make selectrum--read private and deprecate selectrum-read in favor of selectrum-completing-read
- Refactor selectrum--read as you see fit :)
There was a problem hiding this comment.
I did it this way because I want the user option to have precedencoe. But I'm starting to think maybe we should just wait with this and make selectrum--read private first so we do it all in one go.
|
Done in #441 |
@minad I'm not still not sure I would like selectrum metadata, I failed to find the discussion where you brought up arguments why it would be preferable to you, for now I made the variable public you are using public at least.