Cleanup variable usage #441
Conversation
selectrum.el
Outdated
| @@ -88,6 +75,11 @@ See `minibuffer-default-in-prompt-regexps', from which this is derived.") | |||
| :prefix "selectrum-" | |||
| :link '(url-link "https://github.com/raxod502/selectrum")) | |||
|
|
|||
| (defcustom selectrum-should-sort-p t | |||
There was a problem hiding this comment.
Should this really be a defcustom? I think most of the variables which are supposed to be locally tuned in the minibuffer-with-setup-hook should be defvar-local. The same applies to selectrum--move-default-candidate for example. Is this global for legacy reasons? Then I would probably keep it as a defvar, add a comment that this variable is to be deprecated. And later on deprecate the variable, replacing it by a local variant. At the same time one could get rid of the -p suffix which goes against the elisp style guide.
There was a problem hiding this comment.
I think it should be a defcustom the variables are able to be set locally but first and foremost are meant as user options. The name is problematic but I would prefer to avoid inconveniences for this reasons
There was a problem hiding this comment.
Okay, I am only asking for this reason - if it is really intended as a user option, fine! But I still have my doubts that it is useful as a global user option. Same applies to selectrum-move-default-candidate, which you will probably make public at some point (#333).
There was a problem hiding this comment.
I know about users which want to be able to disable sort by default that is why I think it should be an user option. For selectrum-move-default-candidate, I don't think it should be an option as its meant for session configuration.
selectrum.el
Outdated
|
|
||
| (defvar selectrum--candidates-overlay nil | ||
| "Overlay used to display current candidates.") | ||
|
|
||
| (defvar selectrum--dynamic-candidates nil | ||
| (defvar selectrum--count-overlay nil |
There was a problem hiding this comment.
The overlay could also be local since it is tied to a specific recursive minibuffer session I suppose?
There was a problem hiding this comment.
May apply to both overlays? candidates and count?
There was a problem hiding this comment.
We use move-overlay it shouldn't be set locally.
There was a problem hiding this comment.
You probably looked at the old version? This PR handles them both equally
There was a problem hiding this comment.
While I have seen the move-overlay, I have only a superficial picture here. But this does not sound like a good design decision. Overlays are tied to a buffer (see the function overlay-buffer). When going into minibuffer recursion it is probably better to create a separate overlay for each minibuffer buffer. I am also using buffer-local overlays in Consult and that approach seems to work well.
There was a problem hiding this comment.
Icomplete seems to also have this updating bug btw. Probably for the same reason.
There was a problem hiding this comment.
One could add to the README comparison list - Selectrum handles recursive sessions properly ;)
There was a problem hiding this comment.
Yes, seems likely now. I don't often use recursive sessions that is why I never observed it I guess. So this is a clear win for your isolation argument.
There was a problem hiding this comment.
With recursive sessions I feel it would be much nicer if Emacs would behave like embark-become and simply close the other session. I'm always annoyed when doing something in a recursive session (like opening help buffer from drescribe-function) only to see it closed when exiting the session...
There was a problem hiding this comment.
Well, but then it wouldn't be recursive ;) But I am also not entirely convinced by recursive editing. For example with consult-line one can jump out of the minibuffer, do some editing and jump back. If that's useful or not 🤷♂️
clemera
force-pushed
the
cleanup-variable-usage
branch
2 times, most recently
from
1542ae9
to
a8b6617
Compare
|
@minad Added |
|
Okay, cool. I will push the update soon! |
|
From my side it is okay if you remove the private variable right away. I will push the update as soon as you merge this such that we are in sync. Even if someone uses selectrum/consult out of sync, there won't be big issues when consult sets a non-existing private variable. I mean this is only a superficial ui issue. |
clemera
force-pushed
the
cleanup-variable-usage
branch
from
a8b6617
to
278ae49
Compare
clemera
force-pushed
the
cleanup-variable-usage
branch
from
dcb3544
to
72eb04b
Compare
clemera
force-pushed
the
cleanup-variable-usage
branch
from
4b6b7a1
to
fc15e0a
Compare
clemera
force-pushed
the
cleanup-variable-usage
branch
from
c44557d
to
1f8b242
Compare
|
I saw you renamed the sort variable. Do you also plan to rename the other |
clemera
force-pushed
the
cleanup-variable-usage
branch
from
9ff2254
to
6734654
Compare
|
Thanks! I am not a fan of the |
|
No problem originally I wouldn't have renamed all of them here but why not better to be consistent right away. BTW is there some convention at what time removing deprecated aliases is okay?
Do you use something else? |
I don't think there is a commonly accepted guideline. My suggestion would be to deprecate everything in 3.1 and depending on how things go either publish a 3.2, 3.3 or go directly to 4.0, where you remove all the deprecations. This means the removal is not based on time but on the version, which is maybe not the best way given that versions are handled cavalierly in the Emacs ecosystem, e.g. MELPA. However since there are also no dependencies on Selectrum in MELPA, I think it would be okay to do this. There are only few end users who are using the Selectrum APIs directly and I think they are following the project closely.
I usually avoid such meaningless words like |
I already deprecated them here for 3.0, I will probably remove then in 4.0 then I don't plan to do intermediate releases as I'm to lazy for that ;)
Hm for me the main benefit is to quickly identify them as a flag but I agree that usually it is not necessary. I will probably use the |
Yes. What I meant is that the current version is 3.1-devel or something like this, since 3.0 has been released. My suggestion would be to tag a stable 3.1 release soon. And just before tagging the 4.0 version, remove all the deprecations. If the next version is directly 4.0 you will have the deprecations around much longer. So from the laziness point of view there is no difference, you just call the next version 3.1 instead of 4.0. But it seems there haven't been any 1.2 etc releases in Selectrum so in the end it does not really matter. Selectrum seems to be using the versioning style where the major version is incremented quickly, which I am personally not fond of. For example I would consider the version 1.0 the first "really stable" version. But Selectrum is already 3.0 while I would rather consider it to be 0.13 or something like that, given the many internal significant changes you made recently and given how young the project is. Maybe after having reworked all the internals as you did now, I feel the library is much closer to my perceived 1.0 now ;)
The elisp style guide seems to agree with you. I think it is okay. |
|
With my own projects I also tend do wait much longer before an 1.0 release but I have never been into serious versioning until now hence also my hesitation to dealt with it, it's not just about tagging I also need to copy things (changelog/releasepage) around ;) I don't understand what you mean by having the deprecations around longer, I thought I would just tag the 4.0 release and remove the deprecations at the same time. |
|
I think I get your point now: From a release standpoint the depreactions where never within a official release so when this is an requirement there would have to be an intermediate release. |
Exactly. |
|
I see it as follows - the current master branch is still somehow 3.0 release compatible. This means if you release 3.1 with the deprecations, things are still fine and stable users would get warnings. After that the next 4.0 release would then break things. But given that nobody really relies on the versions here, it will be fine either way. You can even kick out stuff without deprecations without many people noticing, but it is just not a good style ;) In Consult since I am still on version 0.5, I am moving things around from time to time until I reach 1.0. But I try to make it a bit carefully and also introduce deprecations if I see fit. I am also releasing rather often, since I don't care if I end up with versions like 0.13 after a while. At that point I can then make a 1.0 release out of it. |
|
This sounds good to me, as I said I'm not very familiar with this process and I don't really know how to do what you are suggesting. I understand that I can tag a past commit as 3.1 but how do I get the depreacations in there? |
|
I would not backport any deprecations, I would do it the lazy way. Let's not bother with this too much now. I suggest you continue as is - I guess there are more deprecations incoming (if we go forward for example with hiding selectrum--read). After having done all this, adapt the changelogs etc and tag the current commit as 3.1. If you want you can also tag some of the next commits as 3.1, and then add more deprecations afterwards. Then later on tag a commit as 3.2. And then just before tagging the 4.0 commit remove all the deprecations. That would also be fine. I don't think there is any magic with the versioning/release process. I would release more often with smaller increments and drop the deprecations when doing the major bumps. That's all. |
|
The problem I see with that is that there are already breaking changes on master so using SemVer the next version should be 4.0 as I understand it. Thanks for your help with this! |
|
I reread your comment above and you are right that it is somewhat compatible, maybe I should do just that. |
|
Agree, I just checked the changelog and would probably take a relaxed stance regarding the current changes. I think making the next version 3.1 is fine. |
|
I just saw - the readme still mentions this save-global-state macro. |
|
Thanks, fixed by 1ba7c3f |
No description provided.