Hi Nicolas, Nicolas Goaziou <m...@nicolasgoaziou.fr> writes:
> Adam Porter <a...@alphapapa.net> 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.
