Derek Chen-Becker <de...@chen-becker.org> 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. > +(defun org-priority-valid-cookie-p (priority) > + "Return t if the PRIORITY is a valid priority cookie, nil otherwise." 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. > + (interactive "P") 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. > +(defun org-priority-value-in-range-p (priority) > + "Return t if the PRIORITY is a value that is in the range of > + org-priority-lowest <= PRIORITY <= org-priority-highest." Note that this docstring does not follow the conventions: (1) First line should be a complete sentence; (2) Symbols inside docstring should be `quoted'. 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? > + ;; validation checks on our current range of priority values > + (unless (org-priority-valid-value-p org-priority-lowest) > + (user-error "Invalid org-priority-lowest: %s" org-priority-lowest)) > + (unless (org-priority-valid-value-p org-priority-highest) > + (user-error "Invalid org-priority-highest: %s" org-priority-highest)) > + (unless (or (and (org-priority-number-p org-priority-highest) > + (org-priority-number-p org-priority-lowest)) > + (and (not (org-priority-number-p org-priority-highest)) > + (not (org-priority-number-p org-priority-lowest)))) > + (user-error "Priority range highest/lowest must both be numeric or > both be alphabetic: %s-%s" > + org-priority-highest > + org-priority-lowest)) 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. > + (s (if (and is-numeric-priority > + (< 9 org-priority-lowest)) > (read-string msg) > - (message msg) Any reason you remove this message? > + ;; The user might have given us garbage > + (unless (org-priority-valid-value-p new-value) > + (user-error "Invalid priority: `%s'" new-value)) > + ;; If we're using alphabetical priorities, we will force uppercase > letters > + (when (not is-numeric-priority) > + (setq new-value (upcase new-value))) 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. > + ;; 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. > --- a/testing/lisp/test-org.el > +++ b/testing/lisp/test-org.el > @@ -1,4 +1,4 @@ > -;;; test-org.el --- tests for org.el -*- lexical-binding: t -*- > + ;;; test-org.el --- tests for org.el -*- lexical-binding: t -*- Stray whitespace change. > From 5c6c1dd28b6447d24918412c1568a8317ac38c6c Mon Sep 17 00:00:00 2001 > From: Derek Chen-Becker <o...@chen-becker.org> > Date: Tue, 29 Jul 2025 06:19:32 -0600 > Subject: [PATCH 1/2] lisp/org.el: Remove deprecated show command > > * org.el (org-priority): Remove the deprecated show command now that we're > several versions beyond when the deprecation was introduced. > --- > lisp/org.el | 6 +----- > 1 file changed, 1 insertion(+), 5 deletions(-) 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). -- Ihor Radchenko // yantar92, Org mode maintainer, Learn more about Org mode at <https://orgmode.org/>. Support Org development at <https://liberapay.com/org-mode>, or support my work at <https://liberapay.com/yantar92>
