>> Why did you recombine everything back into a single function? > >Fair question. I belive your refactor can be broken down into two >important steps. > >1. Switch everything over to org-element api > >2. Split things out into reusable functions >
This is not what you said in your original email. You said: > 1. A code refactor and cleanup that does not add any extension machinery > 2. Add the extension machinery > 3. Add another habit style >Now doing a refactor properly and ensuring that the code does exactly >what it used to do before is pretty difficult. In fact, I'm pretty sure >that with the patch I sent applied, that the code likely acts slightly >different then before. I can't find any specific examples, but I would >bet that they do exist. > >The last thing I want to do is break someone's current working setup. > >The previous maintainer Bastien (and I assume the current maintainer) >are adamant that we don't break the user experience. Here is his blog >post to prove it: > >https://bzg.fr/en/notes/the-software-maintainers-pledge/ I understand this. With any change there is obviously a risk that a user's configuration will be broken. The best way to minimize that risk is testing. That is why I wrote new tests for org-habit. I actually wrote the tests that included my patch before I started refactoring the code in order to lock in the current behavior. You told me to remove those tests. I'm ok with that. What I don't understand is how are we supposed to ensure that our code doesn't break the user experience if we aren't allowed to test internal functions or undocumented behaviors? That's not a question for you obviously, that's a general question for the list. I have attached a patch to this email that addresses all of your critiques. I combined everything back into a single function. I reduced my line lengths. I added back the original state change notes regex. Finally, I removed `org-habit--repeater-unit-to-days'. I am willing to keep working with you on this. I am willing to take critiques. However, I don't think it makes sense for me to review your patch on my thread. I started this thread fully expecting a code review. If you would like to do this refactor yourself, please let me know. I will cancel this thread so that you can start another one. Le dim. 7 juin 2026 à 16:00, Morgan Smith <[email protected]> a écrit : > Earl Chase <[email protected]> writes: > > > Why did you recombine everything back into a single function? > > Fair question. I belive your refactor can be broken down into two > important steps. > > 1. Switch everything over to org-element api > > 2. Split things out into reusable functions > > Now doing a refactor properly and ensuring that the code does exactly > what it used to do before is pretty difficult. In fact, I'm pretty sure > that with the patch I sent applied, that the code likely acts slightly > different then before. I can't find any specific examples, but I would > bet that they do exist. > > The last thing I want to do is break someone's current working setup. > > The previous maintainer Bastien (and I assume the current maintainer) > are adamant that we don't break the user experience. Here is his blog > post to prove it: > > https://bzg.fr/en/notes/the-software-maintainers-pledge/ >
From 7c9e1ba4e07a176a22cef317d0ed2bdbe95112a4 Mon Sep 17 00:00:00 2001 From: ApollonDeParnasse <[email protected]> Date: Sun, 31 May 2026 13:10:27 -0500 Subject: [PATCH] org-habit.el: `org-habit-parse-todo' refactor * lisp/org-habit.el (org-habit-parse-todo): Use the org-element API to get habit data. --- lisp/org-habit.el | 189 ++++++++++++++++++++++++++++++---------------- 1 file changed, 124 insertions(+), 65 deletions(-) diff --git a/lisp/org-habit.el b/lisp/org-habit.el index 8d0108639..dde538f57 100644 --- a/lisp/org-habit.el +++ b/lisp/org-habit.el @@ -35,6 +35,9 @@ (require 'org) (require 'org-agenda) +(declare-function org-element-property "org-element-ast" (property node)) +(declare-function org-element-end "org-element" (node)) + (defgroup org-habit nil "Options concerning habit tracking in Org mode." :tag "Org Habit" @@ -160,6 +163,7 @@ means of creating calendar-based reminders." :group 'org-faces) (defun org-habit-duration-to-days (ts) + (declare (obsolete nil "10.0")) (if (string-match "\\([0-9]+\\)\\([dwmy]\\)" ts) ;; lead time is specified. (floor (* (string-to-number (match-string 1 ts)) @@ -181,69 +185,124 @@ Returns a list with the following elements: 1: \".+\"-style repeater for the schedule, in days 2: Optional deadline (nil if not present) 3: If deadline, the repeater for the deadline, otherwise nil - 4: A list of all the past dates this todo was mark closed - 5: Repeater type as a string - -This list represents a \"habit\" for the rest of this module." - (save-excursion - (if pom (goto-char pom)) - (cl-assert (org-is-habit-p (point))) - (let* ((scheduled (org-get-scheduled-time (point))) - (scheduled-repeat (org-get-repeat (org-entry-get (point) "SCHEDULED"))) - (end (org-entry-end-position)) - (habit-entry (org-no-properties (nth 4 (org-heading-components)))) - closed-dates deadline dr-days sr-days sr-type) - (if scheduled - (setq scheduled (time-to-days scheduled)) - (error "Habit %s has no scheduled date" habit-entry)) - (unless scheduled-repeat - (error - "Habit `%s' has no scheduled repeat period or has an incorrect one" - habit-entry)) - (setq sr-days (org-habit-duration-to-days scheduled-repeat) - sr-type (progn (string-match "[\\.+]?\\+" scheduled-repeat) - (match-string-no-properties 0 scheduled-repeat))) - (unless (> sr-days 0) - (error "Habit %s scheduled repeat period is less than 1d" habit-entry)) - (when (string-match "/\\([0-9]+[dwmy]\\)" scheduled-repeat) - (setq dr-days (org-habit-duration-to-days - (match-string-no-properties 1 scheduled-repeat))) - (if (<= dr-days sr-days) - (error "Habit %s deadline repeat period is less than or equal to scheduled (%s)" - habit-entry scheduled-repeat)) - (setq deadline (+ scheduled (- dr-days sr-days)))) - (org-back-to-heading t) - (let* ((maxdays (+ org-habit-preceding-days org-habit-following-days)) - (reversed org-log-states-order-reversed) - (search (if reversed 're-search-forward 're-search-backward)) - (limit (if reversed end (point))) - (count 0) - (re (format - "^[ \t]*-[ \t]+\\(?:State \"%s\".*%s%s\\)" - (regexp-opt org-done-keywords) - org-ts-regexp-inactive - (let ((value (cdr (assq 'done org-log-note-headings)))) - (if (not value) "" - (concat "\\|" - (org-replace-escapes - (regexp-quote value) - `(("%d" . ,org-ts-regexp-inactive) - ("%D" . ,org-ts-regexp) - ("%s" . "\"\\S-+\"") - ("%S" . "\"\\S-+\"") - ("%t" . ,org-ts-regexp-inactive) - ("%T" . ,org-ts-regexp) - ("%u" . ".*?") - ("%U" . ".*?"))))))))) - (unless reversed (goto-char end)) - (while (and (< count maxdays) (funcall search re limit t)) - (push (time-to-days - (org-time-string-to-time - (or (match-string-no-properties 1) - (match-string-no-properties 2)))) - closed-dates) - (setq count (1+ count)))) - (list scheduled sr-days deadline dr-days closed-dates sr-type)))) + 4: A list of all the past dates this todo was mark done + 5: Symbol of the repeater type + +This list represents a \"habit\" for the rest of this module. +When POM is non-nil, it should be a marker or point." + (cl-flet* + ((org-habit--convert-timestamp-to-days (timestamp-element) + "Convert TIMESTAMP-ELEMENT into a number of days since the epoch." + (if-let* ((time-string + (org-element-property :raw-value timestamp-element))) + (time-to-days (org-time-string-to-time time-string)) + (error "Habit %s has no scheduled date" + (org-element-property :title timestamp-element)))) + + (org-habit--repeater-unit-to-days (repeater-unit) + "Convert REPEATER-UNIT into a number of days." + (pcase repeater-unit + (`day 1) + (`week 7) + (`month 30.4) + (`year 365.25))) + + (org-habit--get-done-dates-for-todo (headline-element) + "Get the dates a todo with a repeater was marked done. + HEADLINE-ELEMENT should be a headline element with a + TODO and state changes notes." + (org-back-to-heading t) + (let* ((maxdays + (+ org-habit-preceding-days org-habit-following-days)) + (reversed org-log-states-order-reversed) + (search + (if reversed 're-search-forward 're-search-backward)) + (end (org-element-end headline-element)) + (limit (if reversed end (point))) + (count 0) + (done-dates) + (re (format + "^[ \t]*-[ \t]+\\(?:State \"%s\".*%s%s\\)" + (regexp-opt org-done-keywords) + org-ts-regexp-inactive + (let ((value (cdr (assq 'done org-log-note-headings)))) + (if (not value) "" + (concat "\\|" + (org-replace-escapes + (regexp-quote value) + `(("%d" . ,org-ts-regexp-inactive) + ("%D" . ,org-ts-regexp) + ("%s" . "\"\\S-+\"") + ("%S" . "\"\\S-+\"") + ("%t" . ,org-ts-regexp-inactive) + ("%T" . ,org-ts-regexp) + ("%u" . ".*?") + ("%U" . ".*?"))))))))) + (unless reversed (goto-char end)) + (while (and (< count maxdays) (funcall search re limit t)) + (push (time-to-days + (org-time-string-to-time + (or (match-string-no-properties 1) + (match-string-no-properties 2)))) + done-dates) + (setq count (1+ count))) + done-dates)) + + (org-habit--get-repeater-and-deadline-data (timestamp-element) + "Extract repeater and deadline data from TIMESTAMP-ELEMENT. + Returns a list with the following elements: + + 0: Scheduled date for the habit (may be in the past) + 1: \".+\"-style repeater for the schedule, in days + 2: Optional deadline (nil if not present) + 3: If deadline, the repeater for the deadline, otherwise nil." + (let* ((scheduled-date-in-days + (org-habit--convert-timestamp-to-days timestamp-element)) + (repeater-unit + (org-element-property :repeater-unit timestamp-element)) + (repeater-value + (org-element-property :repeater-value timestamp-element)) + (repeater-value-in-days + (* repeater-value (org-habit--repeater-unit-to-days repeater-unit))) + (deadline-unit + (org-element-property :repeater-deadline-unit timestamp-element)) + (deadline-value + (org-element-property :repeater-deadline-value timestamp-element)) + (deadline-value-in-days + (when deadline-value (* deadline-value (org-habit--repeater-unit-to-days deadline-unit)))) + (deadline-date-in-days + (when (and deadline-value deadline-value-in-days) + (+ scheduled-date-in-days + (- deadline-value-in-days repeater-value-in-days))))) + (cond + ((<= repeater-value-in-days 0) + (error "Habit %s scheduled repeat period is less than 1d" + (org-element-property :title timestamp-element))) + ((and deadline-value-in-days + (<= deadline-value-in-days repeater-value-in-days)) + (error "Habit %s deadline repeat period is less than or equal to scheduled (%s)" + (org-element-property :title timestamp-element) + repeater-value-in-days)) + (t (list scheduled-date-in-days + repeater-value-in-days + deadline-date-in-days + deadline-value-in-days)))))) + (save-excursion + (if pom (goto-char pom)) + (cl-assert (org-is-habit-p (point))) + (let* ((headline-element (org-element-at-point)) + (scheduled-timestamp + (org-element-property :scheduled headline-element)) + (repeater-type + (org-element-property :repeater-type scheduled-timestamp)) + (repeater-and-deadline-data (if repeater-type + (org-habit--get-repeater-and-deadline-data scheduled-timestamp) + (error "Habit `%s' has no scheduled repeat period or has an incorrect one" + (org-element-property :title headline-element)))) + (done-dates + (org-habit--get-done-dates-for-todo headline-element))) + (append repeater-and-deadline-data + (list done-dates repeater-type)))))) (defsubst org-habit-scheduled (habit) (nth 0 habit)) @@ -363,8 +422,8 @@ current time." ;; At the last done date, use current ;; scheduling in all cases. ((null done-dates) scheduled) - ((equal type ".+") (+ last-done-date s-repeat)) - ((equal type "+") + ((equal type 'restart) (+ last-done-date s-repeat)) + ((equal type 'completed) ;; Since LAST-DONE-DATE, each done mark ;; shifted scheduled date by S-REPEAT. (- scheduled (* (length done-dates) s-repeat))) -- 2.54.0
