On Sat, Sep 27, 2025 at 7:24 AM Ihor Radchenko <[email protected]> wrote:
> Derek Chen-Becker <[email protected]> writes: > > > (org-priority): Refactor to apply more validation and use the new > > predicates and functions defined in this commit to simplify logic and > make > > things more consistent, particularly around double-digit numeric values. > > It would be helpful if you listed what exactly changed compared to > previous situation. > OK, let me try a different approach to how I detail the changes. > If I just look at the functions names `org-priority-valid-cookie-p' and > `org-priority-valid-value-p', I cannot easily tell how "cookie" is > different from "value". At least, the function names should be more > intuitive. Or maybe we can even merge the two functions into 1. > "Cookie" is the term used throughout the rest of the codebase for the string with open/close square brackets (e.g. "[#A]"), as opposed to "value" which is the part inside the brackets following the hash). I was trying to be consistent with existing naming. I'm open to suggestions for alternative nomenclature, but it seemed like a well-established pattern. > Why is the function interactive? I do not see how it can be used by > users in any useful way. Same for all other predicate functions you added. > Mostly for testing on my part, I can remove this. For my own knowledge, is there a performance or other drawback for functions that are marked "interactive" over ones that aren't, or is this just a stylistic concern? > Also, the function name reads weird. Usually, when I see "in-range", I > expect the range to be passed as argument. More importantly, I am not > sure why this should be distinct from `org-priority-valid-value-p'. If > the value is outside the lowest/highest priority, is it valid? > This was actually needed for a later part of the code that you commented on, so I'll continue that discussion around the wrapping check. > It is not a job for runtime to check user customization. I think that it > will be better to write a proper defcustom type specification, so that > Emacs automatically warn users about invalid customization value. > Do you have an example of a defcustom that does that? I tried looking at the docs for defcustom and didn't see an obvious way to do that. I also couldn't find an example in the codebase that seemed to do any validation. > > + (s (if (and is-numeric-priority > > + (< 9 org-priority-lowest)) > > (read-string msg) > > - (message msg) > > Any reason you remove this message? > It looked like a leftover debug statement added in https://cgit.git.savannah.gnu.org/cgit/emacs/org-mode.git/commit/lisp/org.el?id=b91e934a59f0da839c7fc5680d45ce1ddc1b718a. There's no comment explaining why we would echo back what a user just entered to them. and the message was not there prior to the change where we ask for a single character rather than a string. Is Bastien still around so we can ask him? > This reads slightly confusing as it is a hidden feature where passing ?a > when priority range is, say ?A..?C will automatically convert it to ?A. > We should probably explicitly say this in the docstring. > This is only intended to apply to a value where the user has interactively provided a new value via the interactive read of a string, and it matches what we were doing already in this code, which is not commented but effectively was checking to see if we're using alphabetic priorities, and if so converts to upper case: (when (and (= (upcase org-priority-highest) org-priority-highest) (= (upcase org-priority-lowest) org-priority-lowest)) (setq new (upcase new))) This should not have any effect on a priority value passed to the function. > > > + ;; Check if we need to wrap > > + (when (not (org-priority-valid-value-p new-value)) > > But does it check for wrapping? > `org-priority-valid-value-p' checks for the whole 0..64,?A..?Z range, > not for wrapping around the allowed range. > Good call, this is where I would need the predicate that checks for a value within the valid range. Do you have a suggestion for a different name for that predicate than org-priority-value-in-range-p? > When we remove deprecated command, we should announce it in the news. > It is a breaking change (even if it has been announced in the past). > OK, how do I make that announcement? It don't see a NEWS file. Thanks for the feedback! Cheers, Derek -- +---------------------------------------------------------------+ | Derek Chen-Becker | | GPG Key available at https://keybase.io/dchenbecker and | | https://pgp.mit.edu/pks/lookup?search=derek%40chen-becker.org | | Fngrprnt: EB8A 6480 F0A3 C8EB C1E7 7F42 AFC5 AFEE 96E4 6ACC | +---------------------------------------------------------------+
