Hello, Dan Drake <dan.dr...@gmail.com> writes:
> Thank you, Nicolas, for the review of my first patch. I've updated my code > and have the new patch attached. Thank you! > I didn't inline the "time to minutes to keep function"; yes, that function > is very short, but the function name makes the intent/purpose of the code > obvious, and the org-clock-resolve a bit more readable. If it's important > to inline that -- performance concerns about the function call overhead, > accepting org-mode/emacs coding style, etc. -- I'm willing to do that. If you think it makes the code more readable, which is debatable IMO, one option is to simply add a comment where it is going to be inlined. It is not about speed or Org coding style, but generally speaking, defining a global function out of a one-liner, just to use it once, does not sound reasonable. You may define it in a `let' within the function, but I'm still confident that a comment and plain inlining is more than enough. > I added an entry to ORG-NEWS for version 9.4. Great! > I didn't write any tests, as the change seems so simple, and UI-focused. > But again, if that's a dealbreaker, I can work on doing that. It's certainly not a deal breaker in this case, but a regression test more or less guarantees a future code refactoring will not unwillingly break your feature. Test can come later, or even never, that's your call. No worries. Regards, -- Nicolas Goaziou