[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

From: Nicolas Goaziou
Subject: Re: [RFC PATCH] specify a time, not number of minutes to keep, with org-resolve-clock
Date: Wed, 29 Jan 2020 10:49:05 +0100
User-agent: Gnus/5.13 (Gnus v5.13) Emacs/26.3 (gnu/linux)


Dan Drake <address@hidden> 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

> I added an entry to ORG-NEWS for version 9.4.


> 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.


Nicolas Goaziou

reply via email to

[Prev in Thread] Current Thread [Next in Thread]