Thanks for the feedback! I've attached the updated patches. One note on the comments:
> + (<= priority org-priority-lowest) > + (>= priority org-priority-highest))) (<= org-priority-lowest priority org-priority-highest) needs to be (>= org-priority-lowest priority org-priority-highest) because org-priority-lowest > org-priority-highest. Cheers, Derek On Sat, Sep 13, 2025 at 8:39 AM Rudolf Adamkovič <[email protected]> wrote: > Derek Chen-Becker <[email protected]> writes: > > > If this looks reasonable I'll start working on the other parts of the > > codebase that deal with priorities, using the same helper functions. > > Below are some comments from me. > > > +must all be a positive integer between 0 and 64, inclusive. > > "Positive integer between 0 and 64, inclusive" makes zero sense (pun > intended). I think you meant to write a non-negative integer. Zero is > not positive, yet it is listed and the sentence says "inclusive". > > > +Latin alphabetical characters A-Z, and positive integers in between 0 > > +and 64, inclusive. > > Ditto. > > > +The value of the priority cookie must be a capital latin > ^^^^^ > Latin > > > +alphabetic character, A-Z, or can be an integer value in > > +the range 0-64. > ^^^^ > 0 through 64 (perhaps) > "through" implies "inclusive", unlike "to" > > > +(defun org-priority-valid-cookiep (priority) > ^^^^^^^ > cookie-p > > > +(defun org-priority-valid-valuep (priority) > ^^^^^^ > value-p > > > +(defun org-priority-value-in-rangep (priority) > ^^^^^^ > range-p > > > + "Return t if the PRIORITY is a valid priority value that is in the > ^^^^^^^ > useless words > > > + org-priority-lowest >= PRIORITY >= org-priority-highest." > ^^ ^^ > <= <= > > > + (<= priority org-priority-lowest) > > + (>= priority org-priority-highest))) > > (<= org-priority-lowest priority org-priority-highest) > > > +(defun org-priority-numberp (priority) > ^^^^^^^ > number-p > > > + "Return t if the PRIORITY is a number between 0 and 64, inclusive, > ^^^^^^ > integer > > > + (>= priority 0) > > + (<= priority 64))) > > (<= 0 priority 64) > > > +uppercase alphabetic character (A-Z), or an integer between [0,64], > ^^^^^^^ > in > > > + (let ((is_numeric_priority (org-priority-numberp > ^^^^^^^^^^^^^^^^^^^ > is-numeric-priority > > > + (setq remove t new_value ?\ )) > ^^^^^^^^^ > new-value > > > + (setq new_value_string (org-priority-to-string new_value)) > ^^^^^^^^^^^^^^^^ > new-value-string > > > + (if has_existing_cookie > ^^^^^^^^^^^^^^^^^^^ > has-existing-cookie > > + ) > ^ > lonely parens (in the tests) > > Rudy > -- > "If you're thinking without writing, you only think you're thinking." > --- Leslie Lamport > > Rudolf Adamkovič <[email protected]> [he/him] > http://adamkovic.org > -- +---------------------------------------------------------------+ | 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 | +---------------------------------------------------------------+
From d73323e2cd1a8592f185ba2da845882fd8985378 Mon Sep 17 00:00:00 2001 From: Derek Chen-Becker <[email protected]> Date: Sat, 6 Sep 2025 07:07:34 -0600 Subject: [PATCH 2/2] lisp/org.el: Add proper support for numeric priorities * doc/org-manual.org: Update the manual to clarify the allowed values for priorities. * lisp/org.el (org-priority-regexp): Update the priority cookie regular expression to fully validate numeric values, and update the documentation to match. (org-priority-valid-cookiep, org-priority-valid-valuep, org-priority-value-in-rangep, org-priority-numberp): Add validation predicates to test priority cookies and priority values. (org-priority-to-string): Add a function to provide a consistent string value from a given priority value. (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. * testing/lisp/test-org.el: Add unit tests for the new predicats and format functions related to priority. --- doc/org-manual.org | 8 +- lisp/org.el | 163 +++++++++++++++++++++++++++------------ testing/lisp/test-org.el | 66 +++++++++++++++- 3 files changed, 183 insertions(+), 54 deletions(-) diff --git a/doc/org-manual.org b/doc/org-manual.org index 8c6962d4b..a86024cd9 100644 --- a/doc/org-manual.org +++ b/doc/org-manual.org @@ -4626,7 +4626,7 @@ You can also use numeric values for priorities, such as When using numeric priorities, you need to set ~org-priority-highest~, ~org-priority-lowest~ and ~org-priority-default~ to integers, which -must all be strictly inferior to 65. +must all be a non-negative integer between 0 and 64, inclusive. Priorities can be attached to any heading; they do not need to be TODO items. @@ -4661,8 +4661,10 @@ TODO items. #+vindex: org-priority-default You can change the range of allowed priorities by setting the variables ~org-priority-highest~, ~org-priority-lowest~, and -~org-priority-default~. For an individual buffer, you may set these -values (highest, lowest, default) like this (please make sure that the +~org-priority-default~. Valid priority values are single uppercase +Latin alphabetical characters A-Z, and non-negative integers in between 0 +and 64, inclusive. For an individual buffer, you may set these values +(highest, lowest, default) like this (please make sure that the highest priority is earlier in the alphabet than the lowest priority): #+cindex: @samp{PRIORITIES}, keyword diff --git a/lisp/org.el b/lisp/org.el index 602fea261..694454978 100644 --- a/lisp/org.el +++ b/lisp/org.el @@ -11296,14 +11296,64 @@ from the `before-change-functions' in the current buffer." ;;;; Priorities -(defvar org-priority-regexp ".*?\\(\\[#\\([A-Z0-9]+\\)\\] ?\\)" +(defvar org-priority-regexp ".*?\\(\\[#\\([A-Z]\\|[0-9]\\|[1-5][0-9]\\|6[0-4]\\)\\] ?\\)" "Regular expression matching the priority indicator. A priority indicator can be e.g. [#A] or [#1]. +The value of the priority cookie must be a capital Latin +alphabetic character, A through Z, or can be an integer value in +the range 0 through 64. This regular expression matches these groups: 0 : the whole match, e.g. \"TODO [#A] Hack\" 1 : the priority cookie, e.g. \"[#A]\" 2 : the value of the priority cookie, e.g. \"A\".") +(defun org-priority-valid-cookie-p (priority) + "Return t if the PRIORITY is a valid priority cookie, nil otherwise." + (interactive "P") + (cond + ((stringp priority) + (let ((case-fold-search nil)) ;; Force case-sensitive match + ;; (and ... t) to force explicit t/nil + (and (string-match-p org-priority-regexp priority) t))))) + +(defun org-priority-valid-value-p (priority) + "Return t if the PRIORITY is a valid priority value (0-64, A-Z), nil otherwise." + (interactive "P") + ;; Although we can have either numeric or alphabetic priorities, + ;; we simplify here by treating everything as integers because the ASCII + ;; alphabetic range also fits in an integer range. + (and (integerp priority) + (or (and (>= priority 0) + (<= priority 64)) + (and (>= priority ?A) + (<= priority ?Z))))) + +(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." + (interactive "P") + ;; Although we can have either numeric or alphabetic priorities, + ;; we simplify here by treating everything as integers because the ASCII + ;; alphabetic range also fits in an integer range. + (and (org-priority-valid-value-p priority) + (>= org-priority-lowest priority org-priority-highest))) + +(defun org-priority-number-p (priority) + "Return t if the PRIORITY is an integer 0 through 64, inclusive, return + nil otherwise." + (interactive "P") + (and (integerp priority) + (<= 0 priority 64))) + +(defun org-priority-to-string (priority) + "Returns a string form of PRIORITY based on whether it's numeric or alphabetic." + (interactive "P") + (if (org-priority-valid-value-p priority) + (if (org-priority-number-p priority) + (number-to-string priority) + (format "%c" priority)) + (user-error "Invalid priority value `%s'" priority))) + (defun org-priority-up () "Increase the priority of the current item." (interactive) @@ -11320,57 +11370,72 @@ This regular expression matches these groups: When called interactively with a `\\[universal-argument]' prefix, show the priority in the minibuffer instead of changing it. -When called programmatically, ACTION can be `set', `up', `down', -or a character." +When called programmatically, ACTION can be `set', `up', `down', an +uppercase alphabetic character A through Z, or an integer 0 through 64, +inclusive." (interactive "P") (if (equal action '(4)) (org-priority-show) (unless org-priority-enable-commands (user-error "Priority commands are disabled")) + ;; 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)) + ;; If action was not provided, default to "set", which will prompt the user (setq action (or action 'set)) - (let ((nump (< org-priority-lowest 65)) - current new news have remove) + (let ((is-numeric-priority (org-priority-number-p org-priority-lowest)) + current-value new-value new-value-string has-existing-cookie remove) (save-excursion (org-back-to-heading t) (when (looking-at org-priority-regexp) (let ((ms (match-string 2))) - (setq current (org-priority-to-value ms) - have t))) + (setq current-value (org-priority-to-value ms) + has-existing-cookie t))) (cond ((eq action 'remove) - (setq remove t new ?\ )) + (setq remove t new-value ?\ )) + ;; set and a value are treated similarly, but only if the + ;; value is a valid priority ((or (eq action 'set) - (integerp action)) + (org-priority-valid-value-p action)) (if (not (eq action 'set)) - (setq new action) + (setq new-value action) (setq - new - (if nump - (let* ((msg (format "Priority %s-%s, SPC to remove: " - (number-to-string org-priority-highest) - (number-to-string org-priority-lowest))) - (s (if (< 9 org-priority-lowest) + new-value + (let* ((msg (format "Priority %s-%s, SPC to remove: " + (org-priority-to-string org-priority-highest) + (org-priority-to-string org-priority-lowest))) + (s (if (and is-numeric-priority + (< 9 org-priority-lowest)) (read-string msg) - (message msg) - (char-to-string (read-char-exclusive))))) - (if (equal s " ") ?\s (string-to-number s))) - (progn (message "Priority %c-%c, SPC to remove: " - org-priority-highest org-priority-lowest) - (save-match-data - (setq new (read-char-exclusive))))))) - (when (and (= (upcase org-priority-highest) org-priority-highest) - (= (upcase org-priority-lowest) org-priority-lowest)) - (setq new (upcase new))) - (cond ((equal new ?\s) (setq remove t)) - ((or (< (upcase new) org-priority-highest) (> (upcase new) org-priority-lowest)) - (user-error - (if nump - "Priority must be between `%s' and `%s'" - "Priority must be between `%c' and `%c'") - org-priority-highest org-priority-lowest)))) + (char-to-string (read-char-exclusive msg))))) + (if (equal s " ") ?\s (string-to-number s))))) + ;; 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))) + ;; After reading interactive input, set removal flag if needed and + ;; perform validation on the new value + (cond + ((equal new-value ?\s) (setq remove t)) + ((not (org-priority-value-in-range-p new-value)) + (user-error "Priority must be between `%s' and `%s'" + (org-priority-to-string org-priority-highest) + (org-priority-to-string org-priority-lowest))))) ((eq action 'up) - (setq new (if have - (1- current) ; normal cycling + (setq new-value (if has-existing-cookie + (1- current-value) ; normal cycling ;; last priority was empty (if (eq last-command this-command) org-priority-lowest ; wrap around empty to lowest @@ -11379,8 +11444,8 @@ or a character." org-priority-default (1- org-priority-default)))))) ((eq action 'down) - (setq new (if have - (1+ current) ; normal cycling + (setq new-value (if has-existing-cookie + (1+ current-value) ; normal cycling ;; last priority was empty (if (eq last-command this-command) org-priority-highest ; wrap around empty to highest @@ -11389,36 +11454,34 @@ or a character." org-priority-default (1+ org-priority-default)))))) (t (user-error "Invalid action"))) - (when (or (< (upcase new) org-priority-highest) - (> (upcase new) org-priority-lowest)) + ;; Check if we need to wrap + (when (not (org-priority-valid-value-p new-value)) (if (and (memq action '(up down)) - (not have) (not (eq last-command this-command))) - ;; `new' is from default priority + (not has-existing-cookie) (not (eq last-command this-command))) + ;; `new-value' is from default priority (error "The default can not be set, see `org-priority-default' why") - ;; normal cycling: `new' is beyond highest/lowest priority + ;; normal cycling: `new-value' is beyond highest/lowest priority ;; and is wrapped around to the empty priority (setq remove t))) - ;; Numerical priorities are limited to 64, beyond that number, - ;; assume the priority cookie is a character. - (setq news (if (> new 64) (format "%c" new) (format "%s" new))) - (if have + (setq new-value-string (org-priority-to-string new-value)) + (if has-existing-cookie (if remove (replace-match "" t t nil 1) - (replace-match news t t nil 2)) + (replace-match new-value-string t t nil 2)) (if remove (user-error "No priority cookie found in line") (let ((case-fold-search nil)) (looking-at org-todo-line-regexp)) (if (match-end 2) (progn (goto-char (match-end 2)) - (insert " [#" news "]")) + (insert " [#" new-value-string "]")) (goto-char (match-beginning 3)) - (insert "[#" news "] ")))) + (insert "[#" new-value-string "] ")))) (when org-auto-align-tags (org-align-tags))) (if remove (message "Priority removed") - (message "Priority of current item set to %s" news))))) + (message "Priority of current item set to %s" new-value-string))))) (defalias 'org-show-priority 'org-priority-show) (defun org-priority-show () diff --git a/testing/lisp/test-org.el b/testing/lisp/test-org.el index 36dea35b7..014e7593c 100644 --- 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 -*- ;; Copyright (c) David Maus ;; Authors: David Maus @@ -9847,6 +9847,70 @@ two (test-org/extract-mathml-math (org-create-math-formula "quote\" ; |")))))) +;;; Priority validation and handling +(ert-deftest test-org/priority-validation () + "Test validation of priority cookies." + ;; Simple bounds checks on single alphabetic characters + (should + (seq-every-p (lambda (p) + (let ((cookie (format "[#%c]" p))) + (org-priority-valid-cookie-p cookie))) + (number-sequence ?A ?Z))) + ;; Test all valid numbers + (should + (seq-every-p (lambda (p) + (let ((cookie (format "[#%d]" p))) + (org-priority-valid-cookie-p cookie))) + (number-sequence 0 64))) + ;; Invalid characters (not exhaustive) + (should + (not (org-priority-valid-cookie-p "[#$]"))) + ;; Don't accept lower-case + (should + (seq-every-p (lambda (p) + (let ((cookie (format "[#%c]" p))) + (not (org-priority-valid-cookie-p cookie)))) + (number-sequence ?a ?z))) + ;; Invalid numberic values (< 0 or > 64) + (should + (not (org-priority-valid-cookie-p "[#-1]"))) + (should + (not (org-priority-valid-cookie-p "[#65]"))) + ;; Value tests (as opposed to cookie tests) + ;; + ;; Numeric, full range + (should + (let ((org-priority-highest 0) + (org-priority-lowest 64)) + (seq-every-p (lambda (pv) (and (org-priority-valid-value-p pv) + (org-priority-value-in-range-p pv))) + (number-sequence 0 64)))) + ;; Numeric, valid value, but out of range + (should + (let ((org-priority-highest 10) + (org-priority-lowest 20)) + (seq-every-p (lambda (pv) + (and (org-priority-valid-value-p pv) + (not (org-priority-value-in-range-p pv))) + ) + '(0 5 9 21 42 64)))) + ;; ;; Alphabetic, full range + (should + (let ((org-priority-highest ?A) + (org-priority-lowest ?Z)) + (seq-every-p (lambda (pv) (and (org-priority-valid-value-p pv) + (org-priority-value-in-range-p pv))) + (number-sequence ?A ?Z)))) + ;; Alphabetic, valid value, but out of range + (should + (let ((org-priority-highest ?C) + (org-priority-lowest ?K)) + (seq-every-p (lambda (pv) + (and (org-priority-valid-value-p pv) + (not (org-priority-value-in-range-p pv)))) + '(?A ?L ?N ?Z))))) + (provide 'test-org) ;;; test-org.el ends here + -- 2.43.0
From 5c6c1dd28b6447d24918412c1568a8317ac38c6c Mon Sep 17 00:00:00 2001 From: Derek Chen-Becker <[email protected]> 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(-) diff --git a/lisp/org.el b/lisp/org.el index 68ecb95f2..602fea261 100644 --- a/lisp/org.el +++ b/lisp/org.el @@ -11314,7 +11314,7 @@ This regular expression matches these groups: (interactive) (org-priority 'down)) -(defun org-priority (&optional action show) +(defun org-priority (&optional action) "Change the priority of an item. When called interactively with a `\\[universal-argument]' prefix, @@ -11323,10 +11323,6 @@ show the priority in the minibuffer instead of changing it. When called programmatically, ACTION can be `set', `up', `down', or a character." (interactive "P") - (when show - ;; Deprecation warning inserted for Org 9.2; once enough time has - ;; passed the SHOW argument should be removed. - (warn "`org-priority' called with deprecated SHOW argument")) (if (equal action '(4)) (org-priority-show) (unless org-priority-enable-commands -- 2.43.0
