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: Tue, 22 Aug 2017 20:41:12 -0500 [thread overview]
Message-ID: <87a82rkqmf.fsf@alphapapa.net> (raw)
In-Reply-To: 87wp5yir25.fsf@nicolasgoaziou.fr
Hi Nicolas,
Nicolas Goaziou <mail@nicolasgoaziou.fr> writes:
> Nitpick: the paragraph above usually comes after the list of changes.
Fixed.
> Nitpick: you can only write "Use macro" once, on the last line.
Fixed.
> Why `cl-defmacro'? Usually, `defmacro' is enough.
Fixed.
> While we're at it,
>
> `org-add-props' + `copy-sequence' = `propertize'
>
> If you're ready for further refactoring, the nested `if' above could
> be turned into a nicer `cond'.
>
> Same here: `mapc' + `lambda' = `dolist' to avoid funcall overhead. `s'
> could be let-bound too.
I'll refactor these in a separate patch to isolate any potential
mistakes.
>> + ;; When nil, make string automatically and insert it
>> + ((pred null) (insert ,default))))
>
> I suggest to use ,@default (and, obviously (&rest default) in the
> arguments) so we are not limited to one S-exp.
I saw that all of the default header code boiled down to inserting a
string, so I moved the ultimate `insert' into the macro so it could
accept a form that evaluates to a string; it seemed like a cleaner, more
functional approach to essentially accept a string rather than arbitrary
code (even though it does accept any sexp, but that is to avoid
evaluating it unless it's needed). I used a keyword argument rather
than &rest for two reasons: 1) so the macro could accept different
option arguments in the future, and 2) so it's plain from looking at the
macro call that the form given is the default, which may be overridden,
rather than what's always used.
If you would prefer the &rest approach, that would mean doing the actual
insertion into the agenda buffer in the macro call rather than the macro
expansion. I thought it would be better to separate the actual
insertion by abstracting it behind the macro. In fact, it could be done
as a function instead, but, of course, that would mean evaluating the
default even when it's not used, so that's why I chose a macro. (Or the
default could be passed as a lambda, but that seems even more awkward.)
Anyway, let me know what you prefer.
> All in all, the only requested change is `cl-defmacro' -> `defmacro'.
> This is no blocker if you don't want/have time to do the refactoring.
I'll post a new patch soon. Thanks.
next prev parent reply other threads:[~2017-08-23 1:41 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 [this message]
2017-08-23 2:32 ` Adam Porter
2017-08-23 8:48 ` Nicolas Goaziou
2017-09-02 2:41 ` Adam Porter
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=87a82rkqmf.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).