From: Adam Porter <adam@alphapapa.net>
To: emacs-orgmode@gnu.org
Subject: Re: [PATCH] org-agenda: Add 'none setting for org-agenda-overriding-header
Date: Fri, 01 Sep 2017 21:41:52 -0500 [thread overview]
Message-ID: <87efrphle7.fsf@alphapapa.net> (raw)
In-Reply-To: 87lgma1xgj.fsf@nicolasgoaziou.fr
Hi Nicolas,
Nicolas Goaziou <mail@nicolasgoaziou.fr> writes:
> Adam Porter <adam@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.
next prev parent reply other threads:[~2017-09-02 2:42 UTC|newest]
Thread overview: 17+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-08-16 4:20 [PATCH] org-agenda: Add 'none setting for org-agenda-overriding-header Adam Porter
2017-08-17 14:25 ` Nicolas Goaziou
2017-08-17 19:57 ` Adam Porter
2017-08-18 9:07 ` Nicolas Goaziou
2017-08-20 2:47 ` Adam Porter
2017-08-20 8:25 ` Nicolas Goaziou
2017-08-23 1:41 ` Adam Porter
2017-08-23 2:32 ` Adam Porter
2017-08-23 8:48 ` Nicolas Goaziou
2017-09-02 2:41 ` Adam Porter [this message]
2017-09-02 7:49 ` Nicolas Goaziou
2017-09-03 1:44 ` Adam Porter
2017-09-06 11:17 ` Nicolas Goaziou
2017-09-06 23:00 ` Adam Porter
2017-09-10 12:32 ` Nicolas Goaziou
2017-09-06 23:06 ` Adam Porter
2017-08-23 8:37 ` Nicolas Goaziou
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: https://www.orgmode.org/
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=87efrphle7.fsf@alphapapa.net \
--to=adam@alphapapa.net \
--cc=emacs-orgmode@gnu.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://git.savannah.gnu.org/cgit/emacs/org-mode.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).