Derek Chen-Becker <[email protected]> writes: >> 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.
Fair. Still, I think highlighting that cookie is a string would be helpful: `org-priority-valid-cookie-string-p' or so. >> 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? The main drawback is that interactive functions will show up in M-x completions and may confuse users. Emacs generally moves towards making M-x list more specific, only listing the immediately useful commands in current context (see the new `execute-extended-command-for-buffer'). So, adding interactive functions that will not be useful is not a good idea IMHO. >> 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. `org-property-separators'. Also see restricted-sexp customization type described in 15.4.2 Composite Types section of Elisp manual. >> > + (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? There is no echoing back. What is done in first branch of the if is `read-string' that (1) displays MSG; (2) reads user input. The other branch (that you modified) (1) displays MSG; (2) reads user input as character. We display a message to tell user what to input. After your change, "Priority A-C, SPC to remove: " message will not be shown. That's why I ask to list every significant change in the patch. Spelling it out (1) helps yourself to formulate why the change was made; (2) helps other to understand why it was made. >> 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 > ... > This should not have any effect on a priority value passed to the function. But it will, won't it? It is in the same branch of code that assigns the value of ACTION to NEW-VALUE. > ... , 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))) Sure. I do not oppose this. Just wanted you to document this behavior in the docstring explicitly. >> >> > + ;; 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? What about making `org-priority-valid-value-p' check the range as well? If an option to ignore the range is necessary, it can be activated by passing an optional argument. >> 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. /etc/ORG-NEWS -- 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>
