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
