Thank you very much for this patch! I am very excited to get some extension machinery into org-habit!
Earl Chase <[email protected]> writes: > This is a complete refactor of `org-habit-parse-todo'. Well it's a refactor + the addition of some extension machinery. Ideally this would be two patches to make reasoning about this a little easier. > I have also added tests in order to make sure the behavior of > `org-habit-parse-todo' did not change. I do like tests and they are very important. However, we should avoid duplicate tests to keep the test suite maintainable. I belive the tests you've added only test situations that the current suite already tests. Please let me know if this isn't the case. > Going forward, the key HABIT-TYPE can optionally be used > to set the type of a habit. I have assigned the symbol `log-done' to > the current and only habit-type supported by org-habit. > `org-habit--get-rx-for-state-change-notes', > `org-habit--get-done-dates-for-repeater', > `org-habit--parse-todo-with-repeater' and `org-habit-parse-todo' take > a habit-type as one of their arguments. They will only receive > `log-done' for that value until other habit-types are implemented. The > plan is to use those functions for numeric habits. This refactor will > also simplify the process of adding time based habits. I get that you wanted to split things up into smaller functions but I think you might have gone too small. I would put the functionality of `org-habit--get-scheduled-date', `org-habit--get-repeater-deadline-days-and-date', and `org-habit--get-repeater-days-and-type' directly into `org-habit--parse-todo-with-repeater'. The way the code is now is a little hard to follow and the function arguments aren't very well documented (see below). If you later need to split them out you can do that at a later time. Also we should probably deprecate `org-habit-duration-to-days' as you've removed all uses of it. > From 881c039fab3dbdda3f5bfd01b1bb4e499d238050 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 > > +(defun org-habit--repeater-value-to-days (repeater-unit repeater-value) Would it be possible to use `org-agenda-span-to-ndays' instead? > +(defun org-habit--get-scheduled-date (timestamp-element) > + "Get the scheduled date in days for TIMESTAMP-ELEMENT." This gets the absolute number of days of the timestamp argument. Nothing here is "scheduled" specific which is a little confusing. > +(defun org-habit--get-rx-for-state-change-notes (habit-type) > + "Returns a regex for the state changes notes. > +HABIT-TYPE should be `log-done'." > + (let ((headings-key (pcase-exhaustive habit-type > + (`log-done 'done) > + (_ (error "Not implemented yet"))))) > + (format > + "^[ \t]*-[ \t]+\\(?:State \"%s\".*%s%s\\)" > + (regexp-opt org-done-keywords) > + org-ts-regexp-inactive > + (let ((value (alist-get headings-key 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" . ".*?"))))))))) Would it be possible to use `org--log-note-format-regexp' here? Slawomir just added this function very recently. > +(defun org-habit--get-done-dates-for-repeater (element habit-type) > + "Get closed dates from ELEMENT for HABIT-TYPE. > +ELEMENT should be an org-element. An org-element of what type? > +(defun org-habit--parse-todo-with-repeater (element habit-type) > + "Get data from ELEMENT with repeater for HABIT-TYPE. > +ELEMENT should be an org-element. An org-element of what type? > +(defun org-habit--parse-todo (element habit-type) > + "Get data for HABIT-TYPE from ELEMENT. > +ELEMENT should be an org-element. An org-element of what type? I apologize if my criticisms seem harsh. It is not entirely clear to me how you plan to eventually implement other habit styles. I see some of these changes as needlessly complicating some rather straight forward code. I'm sure I'd be more accepting of these changes if I could understand your vision. I would really love for you to send me 3 patches. 1. A code refactor and cleanup that does not add any extension machinery 2. Add the extension machinery 3. Add another habit style If you send me a simple refactor I can approve and apply it right away. If you send me a patch adding extension machinery without an example that uses it, I am very unlikely to accept it. Thank you very much for working on this and I hope you persevere and see this through! Morgan
