Re: [O] [PATCH] Allow early-warning anniversaries in agends [was: Re: org-bbdb-birthday reminder]

[Nicolas Goaziou]

Hello,

Nick Dokos <address@hidden> writes:

> Here's a patch to add a new function to org-bbdb.el that provides early
> warning for upcoming anniversaries; it also adds some documentation to
> org.texi.

Thank you. Some comments follow.

> +(defun org-bbdb-date-list (date n)
> +  "Return a list of dates in (m d y) format from the given 'date' to n-1 
> days hence."
> +  (let ((abs (calendar-absolute-from-gregorian date))
> +        ret)
> +    (reverse (dotimes (i n ret)
> +            (setq ret (cons (calendar-gregorian-from-absolute (+ abs i)) 
> ret))))))

  (dotimes (i n (nreverse ret))
    (push (calendar-gregorian-from-absolute (+ abs i)) ret))

> +
> +;;;###autoload
> +(defun org-bbdb-anniversaries-future (&optional n)
> +  "Return list of anniversaries for today and the next n-1 days (default 
> n=7)."
> +  (if (not n) (setq n 7))

  (let ((n (or n 7)))
    (when (<= n 0) (error "..."))
    (let* (...)))
 
> +  (if (<= n 0) nil
> +    (let* (
> +        ;; list of relevant dates.

You need to capitalize comments.

> +        (dates (org-bbdb-date-list date n))
> +        ;; Function to annotate text of each element of l with the 
> anniversary date d.
> +        (annotate-descriptions
> +         (lambda (d l)
> +           (let ((modify-description
> +                  (lambda (x)
> +                    ;; The assumption here is that x is a bbdb link of the 
> form
> +                    ;; [[bbdb:name][description]]
> +                    ;; This function rather arbitrarily modifies the 
> description
> +                    ;; by adding the date to it in a fixed format.

Missing full stop in first sentence.

> +                    (string-match "]]" x)
> +                    (replace-match (format " -- %d-%02d-%02d\\&" (third d) 
> (first d) (second d))
> +                                   nil nil x))))
> +             (mapcar modify-description l))))

Why don't you simply write

  (mapcar (lambda (x) (string-match "]]" x) ...)
          l)

?

> +        ;; Function to generate the list of annotated anniversaries
> +        ;; for the given date d.
> +        (gen-anniversaries
> +         (lambda (d)
> +           (let ((date d))
> +             ;; rebind 'date' so that org-bbdb-anniversaries will be
> +             ;; fooled into giving us the list for the given date
> +             ;; and then annotate the descriptions for that date.

"Rebind".

> +             (funcall annotate-descriptions d (org-bbdb-anniversaries))))))
> +      ;; map the gen-anniversaries function over the dates
> +      ;; and nconc the results into a single list

Missing capitalization and full stop.

> +      (apply 'nconc (mapcar gen-anniversaries dates)))))

#'nconc (there is also `cl-mapcan').

Also, gen-anniversaries is a one-liner. I don't think it deserves its
own name.

IOW, wouldn't it be better to use less levels of indirection, i.e., less
helper functions?


Regards,

-- 
Nicolas Goaziou