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: Sat, 25 Jan 2020 00:30:57 +0100 Message-ID: <87sgk4o2j2.fsf@nicolasgoaziou.fr> References: Mime-Version: 1.0 Content-Type: text/plain Return-path: Received: from eggs.gnu.org ([2001:470:142:3::10]:50596) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1iv8Pu-0001Z6-SZ for emacs-orgmode@gnu.org; Fri, 24 Jan 2020 18:31:12 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1iv8Ps-0003XO-FO for emacs-orgmode@gnu.org; Fri, 24 Jan 2020 18:31:09 -0500 Received: from relay1-d.mail.gandi.net ([217.70.183.193]:26813) by eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32) (Exim 4.71) (envelope-from ) id 1iv8Pq-0003Tj-TX for emacs-orgmode@gnu.org; Fri, 24 Jan 2020 18:31:08 -0500 In-Reply-To: (Dan Drake's message of "Sun, 19 Jan 2020 14:13:41 -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: > I asked about a way to specify a time when using org-resolve-clock instead > of a number of minutes: > > https://lists.gnu.org/archive/html/emacs-orgmode/2020-01/msg00010.html > > I've implemented this myself and a patch is attached. Comments welcome -- > my change works, but I'm not sure about coding style, and right now there's > no error checking. Thank you. Some comments follow. > I marked the patch as a tiny change, but it does add a new menu option and > behavior to org-resolve-clock, so there may be an argument that it's not, > from a user perspective, a "tiny change", but code-wise it's quite simple: > the core logic really isn't more than "ask the user for a time and > subtract". TINYCHANGE is only about the number of non-trivial lines of code in your patch (15 or so). > +(defun time-to-mins-to-keep (start-time) > + "Asks the user for a time and returns the number of minutes > +from START-TIME to that time." > + (floor (/ (float-time > + (time-subtract (org-read-date t t) start-time)) 60))) The name of the function is wrong, it should start with "org-clock". Also, docstring's first line must contain a full sentence. Anyway, this is used only once in your patch, I suggest to inline in instead. > + (or (and (memq ch '(?k ?K)) > + (read-number "Keep how many minutes? " default)) > + (and (memq ch '(?t ?T)) > + (time-to-mins-to-keep last-valid)))) See above about inlining. Would you mind adding an ORG-NEWS entry, and, if possible, writing a test? Regards, -- Nicolas Goaziou