[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [O] [PATCH] org-agenda: Add 'none setting for org-agenda-overriding-

From: Adam Porter
Subject: Re: [O] [PATCH] org-agenda: Add 'none setting for org-agenda-overriding-header
Date: Fri, 01 Sep 2017 21:41:52 -0500
User-agent: Gnus/5.13 (Gnus v5.13) Emacs/25.2 (gnu/linux)

Hi Nicolas,

Nicolas Goaziou <address@hidden> writes:

> Adam Porter <address@hidden> 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

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

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.


reply via email to

[Prev in Thread] Current Thread [Next in Thread]