From mboxrd@z Thu Jan 1 00:00:00 1970 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 Message-ID: <8736byegoe.fsf@nicolasgoaziou.fr> References: <87sgk4o2j2.fsf@nicolasgoaziou.fr> Mime-Version: 1.0 Content-Type: text/plain Return-path: Received: from eggs.gnu.org ([2001:470:142:3::10]:56978) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1iwjyK-0003eY-9S for emacs-orgmode@gnu.org; Wed, 29 Jan 2020 04:49:21 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1iwjyI-0000FM-QE for emacs-orgmode@gnu.org; Wed, 29 Jan 2020 04:49:19 -0500 Received: from relay6-d.mail.gandi.net ([217.70.183.198]:44989) by eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32) (Exim 4.71) (envelope-from ) id 1iwjyI-00007j-KO for emacs-orgmode@gnu.org; Wed, 29 Jan 2020 04:49:18 -0500 In-Reply-To: (Dan Drake's message of "Tue, 28 Jan 2020 20:11:42 -0600") List-Id: "General discussions about Org-mode." List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: emacs-orgmode-bounces+geo-emacs-orgmode=m.gmane-mx.org@gnu.org Sender: "Emacs-orgmode" To: Dan Drake Cc: emacs-orgmode@gnu.org Hello, Dan Drake 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