Re: [RFC PATCH] specify a time, not number of minutes to keep, with org-resolve-clock

[Nicolas Goaziou <mail@nicolasgoaziou.fr>]

Hello,

Dan Drake <dan.drake@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