Ping. Le mer. 17 juin 2026 à 08:47, Earl Chase <[email protected]> a écrit :
> > > > > >>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. > > > > > > 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. > > > > Your tests did not test anything new. Yes your tests are very helpful > during > > development as they move the error closer to where it appears, but the > tests > > are not novel as far as I can tell. > > > > > In your refactor you are doing two things: > > > > 1. Switching the functions to use the org-element API > > > > 2. Moving everything around > > > > > > Both of these individually can create unwanted changes. Many of these > unwanted > > changes can be caught by static analysis (just reading the diff). > However, to > > make this approach efficient, we need focused and small diffs. This is > why I > > have been asking for smaller and smaller commits. > > > > > I do want to make something clear: I want all your changes. I want > org-habit > > to look radically different. I want all the features you want. > However, I > > want each step to be small and easy to understand. > > > > Now testing is useful, but it does not replace static analysis. I mean I > > suppose in theory it could but in this instance it clearly does not. > > > > There are no tests for month/year repeaters and there are no tests for > > malformed data. > > > > I added week, month and years tests. I also added more tests for > malformed data. In the future, it would be good to actually define > errors for malformed data so that we can actually assert the correct > errors are thrown for each type of malformed data. Unless I'm missing > something, I don't know another way to confirm the correct errors are > being thrown for the malformed data tests. Beyond that, I really do > think we need more tests for org-habit. I will focus on expanding the > current tests in the future instead of just adding new ones. > > > What data is accepted is where I suspect the switch to the org-element > API will > > yield different results from the existing code. See below for some > cases I found > > > > This is not to say that we need those tests as we can still catch > problems with > > static analysis. > > > > > > > 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 > > > > You're right. You gave me exactly what I asked for and now I'm asking > for > > something different. I'm really sorry and that must be really > frustrating. > > What I meant, but did not say, is that I would like small focused > changes. > > > > > > > I have attached a patch to this email that addresses all of your > critiques. I > > > combined everything back into a single function. > > > > I am new to review and I aplogize for the confusion. I'm going to need > to > > learn to express myself more clearly and concisly. My issue is not > about how > > many functions there are but is about the size of the diff. > > > > > I added back the original state change notes regex. > > > > I aplogize if I mislead you with the suggestion of > `org-agenda-span-to-ndays' > > and `org--log-note-format-regexp'. My intention was not to offer > solutions but > > to point towards something I thought might be relevant for you to analyze > > further. > > > > > 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. > > > > I don't understand the issue with offering a patch for you to review. I > wanted > > to clearly express the type of patch I wanted to see and figured the > best way > > was to show you exactly what I was looking for. I aplogize if this > offended > > you as that was not the intention. Although I'm still not sure exactly > what > > the issue with doing so is. > > > > If you'd like to request a different reviewer we can make that happen. > > > > I apologize if my tone was too harsh. My point was that typically when > you do a code review, you just review the code. I've never done a code > review where the reviewer just sends me the version they want. I feel > like that defeats the purpose of the whole endeavour. > > > Also I was wondering if using the Emacs builtin "range.el" library would > be > > useful for managing the list of done dates? It seems like a nifty > library. > > > > I'm honestly not a huge fan of "range.el". It's a good idea, but the > execution leaves much to be desired in my opinion. Really, org-mode > needs its own solution for date ranges. In any case, I won't be > touching that code until we start working on the extension mechanism. > > > > +This list represents a \"habit\" for the rest of this module. > > > > At a future point it might be nice to make a "habit-data" cl-defstruct. > > > > Maybe. Just converting the habit-data structure into an alist with > symbol keys, e.g. (list (cons 'scheduled-date 739785) (cons > 'repeater-style 1) ...), would already mean we could drop the defsubst > and just use alist-get or map-elt. > > > > + (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))) > > > > `cl-case' would do here. Doesn't handle 'hour value (explained more > > below). > > > I updated this function so that it errors on hours or when there is no > repeater unit at all. Is there any reason specifically that I should > use `pcase' over `cl-case'? > > > > > > + (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)))) > > > > The existing code searches for repeaters with a regex of "dwmy" whereas > > an org-element repeater can also include a "h" for hour. I believe this > > code will multiply by 'nil' for hour repeaters/deadlines. > > > > Yeah good catch. I mixed up the error handling. I made sure to be more > consistent this time. > > > Also the previous code call `floor' on the value-in-days variables to > > account for floats caused by month and year repeaters. Is that no > > longer necessary? > > > > That was a mistake. I added that back. > > > > + (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)) > > > > I imagine this will give an uninformative error if `scheduled-timestamp' > > is nil > > > > I made sure `org-habit--convert-timestamp-to-days' throws a good error > when there is no 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)))) > > > > Excessively long lines > > > > > > Sorry, I've gotten in the bad habit of writing long lines. > > > Le mar. 9 juin 2026 à 13:50, Morgan Smith <[email protected]> a > écrit : > > > > Earl Chase <[email protected]> writes: > > > > > > > >>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. > > > > > > 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. > > > > Your tests did not test anything new. Yes your tests are very helpful > during > > development as they move the error closer to where it appears, but the > tests > > are not novel as far as I can tell. > > > > In your refactor you are doing two things: > > > > 1. Switching the functions to use the org-element API > > > > 2. Moving everything around > > > > > > Both of these individually can create unwanted changes. Many of these > unwanted > > changes can be caught by static analysis (just reading the diff). > However, to > > make this approach efficient, we need focused and small diffs. This is > why I > > have been asking for smaller and smaller commits. > > > > I do want to make something clear: I want all your changes. I want > org-habit > > to look radically different. I want all the features you want. > However, I > > want each step to be small and easy to understand. > > > > Now testing is useful, but it does not replace static analysis. I mean I > > suppose in theory it could but in this instance it clearly does not. > > > > There are no tests for month/year repeaters and there are no tests for > > malformed data. > > > > What data is accepted is where I suspect the switch to the org-element > API will > > yield different results from the existing code. See below for some > cases I found > > > > This is not to say that we need those tests as we can still catch > problems with > > static analysis. > > > > > > > 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 > > > > You're right. You gave me exactly what I asked for and now I'm asking > for > > something different. I'm really sorry and that must be really > frustrating. > > What I meant, but did not say, is that I would like small focused > changes. > > > > > > > I have attached a patch to this email that addresses all of your > critiques. I > > > combined everything back into a single function. > > > > I am new to review and I aplogize for the confusion. I'm going to need > to > > learn to express myself more clearly and concisly. My issue is not > about how > > many functions there are but is about the size of the diff. > > > > > I added back the original state change notes regex. > > > > I aplogize if I mislead you with the suggestion of > `org-agenda-span-to-ndays' > > and `org--log-note-format-regexp'. My intention was not to offer > solutions but > > to point towards something I thought might be relevant for you to analyze > > further. > > > > > 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. > > > > I don't understand the issue with offering a patch for you to review. I > wanted > > to clearly express the type of patch I wanted to see and figured the > best way > > was to show you exactly what I was looking for. I aplogize if this > offended > > you as that was not the intention. Although I'm still not sure exactly > what > > the issue with doing so is. > > > > If you'd like to request a different reviewer we can make that happen. > > > > Also I was wondering if using the Emacs builtin "range.el" library would > be > > useful for managing the list of done dates? It seems like a nifty > library. > > > > > +This list represents a \"habit\" for the rest of this module. > > > > At a future point it might be nice to make a "habit-data" cl-defstruct. > > > > > + (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))) > > > > `cl-case' would do here. Doesn't handle 'hour value (explained more > > below). > > > > > > > + (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)))) > > > > The existing code searches for repeaters with a regex of "dwmy" whereas > > an org-element repeater can also include a "h" for hour. I believe this > > code will multiply by 'nil' for hour repeaters/deadlines. > > > > Also the previous code call `floor' on the value-in-days variables to > > account for floats caused by month and year repeaters. Is that no > > longer necessary? > > > > > + (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)) > > > > I imagine this will give an uninformative error if `scheduled-timestamp' > > is nil > > > > > + (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)))) > > > > Excessively long lines >
