[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) |
Hello,
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
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
- [RFC PATCH] specify a time, not number of minutes to keep, with org-resolve-clock, Dan Drake, 2020/01/19
- Re: [RFC PATCH] specify a time, not number of minutes to keep, with org-resolve-clock, Nicolas Goaziou, 2020/01/24
- Re: [RFC PATCH] specify a time, not number of minutes to keep, with org-resolve-clock, Dan Drake, 2020/01/28
- Re: [RFC PATCH] specify a time, not number of minutes to keep, with org-resolve-clock,
Nicolas Goaziou <=
- Re: [RFC PATCH] specify a time, not number of minutes to keep, with org-resolve-clock, Bastien, 2020/01/29