From mboxrd@z Thu Jan 1 00:00:00 1970 From: Adam Porter Subject: Re: [PATCH] org-agenda: Add 'none setting for org-agenda-overriding-header Date: Fri, 01 Sep 2017 21:41:52 -0500 Message-ID: <87efrphle7.fsf@alphapapa.net> References: <87k22416un.fsf@alphapapa.net> <87bmnep8yz.fsf@nicolasgoaziou.fr> <8760dmynkk.fsf@alphapapa.net> <871so9nt04.fsf@nicolasgoaziou.fr> <87pobrc5vp.fsf@alphapapa.net> <87wp5yir25.fsf@nicolasgoaziou.fr> <87a82rkqmf.fsf@alphapapa.net> <8760dfko9f.fsf@alphapapa.net> <87lgma1xgj.fsf@nicolasgoaziou.fr> Mime-Version: 1.0 Content-Type: text/plain Return-path: Received: from eggs.gnu.org ([2001:4830:134:3::10]:55386) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1dnyO6-0003ha-LY for emacs-orgmode@gnu.org; Fri, 01 Sep 2017 22:42:23 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1dnyO3-0007XZ-I3 for emacs-orgmode@gnu.org; Fri, 01 Sep 2017 22:42:22 -0400 Received: from [195.159.176.226] (port=43919 helo=blaine.gmane.org) by eggs.gnu.org with esmtps (TLS1.0:RSA_AES_128_CBC_SHA1:16) (Exim 4.71) (envelope-from ) id 1dnyO3-0007XL-Aj for emacs-orgmode@gnu.org; Fri, 01 Sep 2017 22:42:19 -0400 Received: from list by blaine.gmane.org with local (Exim 4.84_2) (envelope-from ) id 1dnyNo-0003no-6D for emacs-orgmode@gnu.org; Sat, 02 Sep 2017 04:42:04 +0200 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: emacs-orgmode@gnu.org Hi Nicolas, Nicolas Goaziou writes: > Adam Porter writes: > >> Here are the patches. Please let me know if any other changes are >> needed. > > Thank you! Comments follow. > >> +(defmacro org-agenda--insert-overriding-header (&key default) > > There is no "&key" in `defmacro'. You're right, that's why IIRC I used cl-defmacro originally. The issue here seems to be using the keyword argument. It seemed like a good idea to specify it with the ":default: keyword argument for two reasons: 1. To make it explicitly clear that the body of code passed to the macro is only the default header, not necessarily the one that will be used. 2. To make it easier to add other arguments in the future. For example, I could imagine adding an argument to change the face or properties of the overriding header, a prefix or suffix, etc., and those would be much easier to handle as keyword args, to avoid calling it as something like "nil nil t nil", as happens with some other Emacs function signatures (e.g. I often have to look up how to call re-search-forward as opposed to replace-regexp-in-string). > It should be (default). > >> + "Insert header into agenda view depending on value of `org-agenda-overriding-header'. >> +If the empty string, don't insert a header. If any other string, >> +insert it as a header. If nil, insert DEFAULT, which should >> +evaluate to a string." >> + (declare (debug (&key form))) > > It needs to be updated according to the above. I'm not sure what you mean here, but I guess it depends on the previous matter. >> + ;; Format week number span >> + (cond ((< (- d2 d1) 350) >> + (if (= w1 w2) >> + (format " (W%02d)" w1) >> + (format " (W%02d-W%02d)" w1 w2))) >> + (t "")) > > (cond ((<= 350 (- d2 d1)) "") > ((= w1 w2) (format " (W%02d)" w1)) > (t (format " (W%02d-W%02d)" w1 w2))) I see. That doesn't seem to be exactly the same logic as the nested if, and I didn't follow the surrounding logic well enough to try to transform it that way. But I assume you know what you're doing, so I'll swap that in. :) >> - (let ((n 0) s) >> - (mapc (lambda (x) >> - (setq s (format "(%d)%s" (setq n (1+ n)) x)) >> - (if (> (+ (current-column) (string-width s) 1) (frame-width)) >> - (insert "\n ")) >> - (insert " " s)) >> - kwds)) >> + (cl-loop for keyword in kwds >> + and num from 1 >> + for string = (format "(%d)%s" num keyword) >> + when (> (+ (current-column) (string-width string) 1) >> + (window-width)) >> + do (insert "\n ") >> + do (insert " " string)) > > Ouch. Why `cl-loop' over `dolist'? Also it looks wrong since the last > `do' is not always executed? (or is it?). Yes, it is always executed: the "when" only applies to the next clause, and I tested it to be sure, both by executing it and expanding the macro. I've used cl-loop a lot lately, so it is familiar to me. > I know there is more than one way to skin a cat, but I'd rather use > a straightforward one: > > (let ((n 0)) > (dolist (k kwds) > (let ((s (format "(%d)%s" (cl-incf n) k))) > (when (> (+ (current-column) (string-width s) 1) (frame-width)) > (insert "\n ")) > (insert " " s)))) I guess this is a matter of style, as I prefer the cl-loop version, which doesn't hide the incrementing in the format call and avoids another level of nesting just for the counter variable. :) But if you want me to use the dolist instead, it's up to you. Let me know about these issues and I'll send another patch series. Thanks.