Why did you recombine everything back into a single function? Le dim. 7 juin 2026 à 13:54, Morgan Smith <[email protected]> a écrit :
> I aplogize for the delay in getting back to you. I think patch review > might be a skill that I don't quite have yet. Let's take a look. > > Earl Chase <[email protected]> writes: > > > Le lun. 1 juin 2026 à 14:26, Morgan Smith <[email protected]> > a écrit : > > > > I removed the new tests I added for org-habit-parse-todo. However, I > think we > > should keep the new tests that I added for > > `org-habit--get-done-dates-for-todo'. I think it's important to be able > to > > test the code that just fetches state change notes in complete > isolation. Those > > tests will also certainly come in handy for the next stage of this > process, > > which is refactoring `org-habit-build-graph'. I think there is a way to > have > > `org-habit--get-done-dates-for-todo' fetch dates for more intelligently > than it > > does now. > > Both Slawomir and I have tried on a few occasions to add tests for internal > functions or for undocumented behavior and had these patches rejected. I > do > see the value and I appreciate your tests. However, let's wait a little > while > to add these tests. Eventually the return value of the function will be > important to end users so we can add the tests when we are closer to that > stage. > > >> Also we should probably deprecate `org-habit-duration-to-days' as you've > >> removed all uses of it. > > > > Done. > > > > (defun org-habit-duration-to-days (ts) > > + (declare (obsolete nil "9.8")) > > (if (string-match "\\([0-9]+\\)\\([dwmy]\\)" ts) > > ;; lead time is specified. > > (floor (* (string-to-number (match-string 1 ts)) > > > The current version is 10.0, not 9.8. See the the top of "lisp/org.el" > where > it says: > > ;; Version: 10.0-pre > > >> > 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? > >> > > > > Done. Honestly I was looking for a function like this originally. I only > wrote > > that because I couldn't find anything. > > > > Looking closer at `org-agenda-span-to-ndays' shows that it is not a simple > drop > in replacement. Unless we provide it START-DAY, it will error on 'month > and > 'year. While we will probably use this function at some point, a simple > refactor is not the right time. > > >> > + (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. > >> > > > > Done. > > > > This does change the regex that comes out of the concat from > > "\\|CLOSING NOTE > \\[\\([[:digit:]]\\{4\\}-[[:digit:]]\\{2\\}-[[:digit:]]\\{2\\}\\(?: > .*?\\)?\\)\\]" > > to > > "\\|CLOSING +NOTE > +\\[\\([[:digit:]]\\{4\\}-[[:digit:]]\\{2\\}-[[:digit:]]\\{2\\}\\(?: > +.*?\\)?\\)\\]" > > > I haven't analyzed it to see if the difference is important, but once again > this would be a behavior change that we probably don't want to include in a > refactor. > > > > > > For the next patch, I am actually going to refactor > `org-habit-build-graph'. > > Extension machinery will be completed in a future patch. > > > > Sounds good to me! I do love a good refactor. > > > Also please watch your line lengths. I think we have an 80 character line > limit and some of your likes are 160+. > > > I combined some of my earlier attempts to refactor org-habit with the > patch you > sent and I have attached it here for you to review. Please let me know > what you think! > >
