From mboxrd@z Thu Jan 1 00:00:00 1970 From: Nicolas Goaziou Subject: Re: [PATCH] Support time units in est+ Date: Sat, 04 Feb 2017 15:30:10 +0100 Message-ID: <87k296xc3x.fsf@nicolasgoaziou.fr> References: <861sw0az19.fsf@gmail.com> Mime-Version: 1.0 Content-Type: text/plain Return-path: Received: from eggs.gnu.org ([2001:4830:134:3::10]:44513) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ca1RT-0006HU-Lx for emacs-orgmode@gnu.org; Sat, 04 Feb 2017 09:35:58 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ca1RQ-000370-Id for emacs-orgmode@gnu.org; Sat, 04 Feb 2017 09:35:55 -0500 Received: from relay2-d.mail.gandi.net ([217.70.183.194]:45129) by eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32) (Exim 4.71) (envelope-from ) id 1ca1RQ-00036t-8A for emacs-orgmode@gnu.org; Sat, 04 Feb 2017 09:35:52 -0500 In-Reply-To: <861sw0az19.fsf@gmail.com> (Malcolm Matalka's message of "Wed, 18 Jan 2017 20:33:38 +0000") 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.org@gnu.org Sender: "Emacs-orgmode" To: Malcolm Matalka Cc: emacs-orgmode@gnu.org Hello, Malcolm Matalka writes: > Hey, this is my first elisp programming so I'm quite certain this is a > big hack. I just stole elements from elsewhere in the file. I'm hoping > this is good enough to get accepted then perhaps someone with more taste > would be able to refactor it to be a bit better. That's not bad. Thank you. > Let me know if I need to change anything. > > From 1167bd20e042ad2ae3f2712f596d76ad8b230336 Mon Sep 17 00:00:00 2001 > From: orbitz > Date: Wed, 18 Jan 2017 21:18:23 +0100 > Subject: [PATCH] org-colview.el: Add support for time units to est+ > > * lisp/org-colview.el: Add support for time units in est+. Ranges are interpreted just like non-range estimates. You need to provide the name of the function being modified: * lisp/org-colview.el (org-columns--summary-estimate): Add support... > > TINYCHANGE > --- > lisp/org-colview.el | 44 ++++++++++++++++++++++++++++++++------------ > 1 file changed, 32 insertions(+), 12 deletions(-) > > diff --git a/lisp/org-colview.el b/lisp/org-colview.el > index 45c71a028..2a5c067ac 100644 > --- a/lisp/org-colview.el > +++ b/lisp/org-colview.el > @@ -1288,23 +1288,43 @@ When PRINTF is non-nil, use it to format the result." > (/ (apply #'+ (mapcar #'org-columns--age-to-seconds ages)) > (float (length ages))))) > > -(defun org-columns--summary-estimate (estimates printf) > +(defun org-columns--summary-estimate (estimates _) > "Combine a list of estimates, using mean and variance. > The mean and variance of the result will be the sum of the means > and variances (respectively) of the individual estimates." Could you expound the docstring? In particular, it should make the output format predictable. See `org-columns--summary-apply-times' for example. > (let ((mean 0) > - (var 0)) > + (var 0) > + (hms-flag nil) > + (duration-flag nil)) > (dolist (e estimates) > - (pcase (mapcar #'string-to-number (split-string e "-")) > -(`(,low ,high) > - (let ((m (/ (+ low high) 2.0))) > - (cl-incf mean m) > - (cl-incf var (- (/ (+ (* low low) (* high high)) 2.0) (* m m))))) > -(`(,value) (cl-incf mean value)))) > - (let ((sd (sqrt var))) > - (format "%s-%s" > - (format (or printf "%.0f") (- mean sd)) > - (format (or printf "%.0f") (+ mean sd)))))) > + (pcase (split-string e "-") > + (`(,low ,high) > + (dolist (time (list high low)) > + (cond > + (duration-flag) > + ((string-match-p org-columns--duration-re time) > + (setq duration-flag t)) > + (hms-flag) > + ((string-match-p "\\`[0-9]+:[0-9]+:[0-9]+\\'" time) > + (setq hms-flag t)))) > + (let* ((low-sec (org-columns--time-to-seconds low)) > + (high-sec (org-columns--time-to-seconds high)) > + (m (/ (+ low-sec high-sec) 2.0))) > + (cl-incf mean m) > + (cl-incf var (- (/ (+ (* low-sec low-sec) (* high-sec high-sec)) 2.0) (* m m))))) > + (`(,value) (cl-incf mean (org-columns--time-to-seconds > value))))) There is much code duplication with `org-columns--summary-apply-times'. It would be better to avoid it. We may want to create a new function turning a list of durations into numbers (with `org-columns--time-to-seconds) and adding the type of the expected output, as a symbol, in front of the list. Then both `org-columns--summary-estimate' and `org-columns--summary-apply-times' could use it. It would mean we would process the list of estimates two times, but I don't think it matters much in practice. If you have an implementation for that, feel free to include it. Do not consider it a blocker, tho. We can always factor this out later. > + (let* ((sd (sqrt var)) > + (low-second (truncate (- mean sd))) > + (high-second (truncate (+ mean sd))) > + (low > + (cond (duration-flag (org-minutes-to-clocksum-string (/ low-second 60.0))) > + (hms-flag (format-seconds "%h:%.2m:%.2s" low-second)) > + (t (format-seconds "%h:%.2m" low-second)))) > + (high > + (cond (duration-flag (org-minutes-to-clocksum-string (/ high-second 60.0))) > + (hms-flag (format-seconds "%h:%.2m:%.2s" high-second)) > + (t (format-seconds "%h:%.2m" high-second))))) > + (format "%s-%s" low high)))) You don't need to `truncate' the estimates. I don't think the default output format should be "%h:%.2m". If no special format is present, I think the traditional "%.0f" can be used. Also, the duplicate `cond' can be factored out. The whole part above can be written (including suggestions aforementioned): (mapconcat (lambda (time) (cond (duration-flag (org-minutes-to-clocksum-string (/ time 60))) (hms-flag (format-seconds "%h:%.2m:%.2s" time)) (t (format "%.0f" time)))) (let ((sd (sqrt var))) (list (- mean sd) (+ mean sd))) "-") It would be nice to add a bunch of tests for various estimates formats. `test-org-colview/columns-summary' in "test-org-colview.el" contains already many examples to boot. Eventually, could you add an entry about this new feature in ORG-NEWS? Regards, -- Nicolas Goaziou