Hi Nicolas, Nicolas Goaziou <m...@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.
