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

[Adam Porter]

Hi Nicolas,

Nicolas Goaziou <address@hidden> 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.