Re: [O] org-review-schedule

[Nicolas Goaziou]

Hello,

Alan Schmitt <address@hidden> writes:

> I changed the date. As I signed the FSF paper, do I need to change the
> name as well and put mine?

For contrib/ directory, you can assign copyright to your name.

> I attach the new version.

Thanks. A few minor comments follow.

> I would like to propose to add this to the contrib directory, but
> I don't know the procedure to submit this code.

You simply copy the file in the contrib/lisp/ directory, edit
contrib/README and edit `org-modules' defcustom in "org.el".

> ;; Example use.
> ;; 

Trailing whitespace.

> ;; 1 - To display the things to review in the agenda.
> ;; 

Ditto.

> (defun org-review-last-planned (last delay)
>   "Computes the next planned review, given the LAST review
>   date (in string format) and the review DELAY (in string
>   format)."
>   (let* ((lt (org-read-date nil t last))
>          (ct (current-time)))

Plain `let' is sufficient here.

>     (time-add lt (time-subtract (org-read-date nil t delay) ct))))
>
> (defun org-review-last-review-prop ()
>   "Return the value of the last review property of the current
> headline."
>   (let ((lr-prop org-review-last-property-name))
>     (org-entry-get (point) lr-prop)))

The `let' seems useless. I think

  (org-entry-get (point) org-review-last-property-name)

is simple enough.

> (defun org-review-toreview-p ()
>   "Check if the entry at point should be marked for review.
> Return nil if the entry does not need to be reviewed. Otherwise return
> the number of days between the past planned review date and today.
>
> If there is no last review date, return nil.
> If there is no review delay period, use `org-review-delay'."
>   (let* ((lr-prop org-review-last-property-name)
>          (lp (org-entry-get (point) lr-prop)))

I suggest to use your own function:


 (let ((lp (org-review-last-review-prop)))
    ...)

>     (when lp 

Trailing whitespace.

>       (let* ((dr-prop org-review-delay-property-name)
>              (dr (or (org-entry-get (point) dr-prop t) 
>                      org-review-delay))

Trailing whitespace.  Also I suggest to merge DR and DR-PROP:

  (let* ((dr (or (org-entry-GET (point) org-review-delay-property-name t)
                org-review-delay))
         ...))

>              (nt (org-review-last-planned lp dr))
>              )

Dangling parenthesis.

>         (if (time-less-p nt (current-time)) nt)))))

This is a matter of taste, but I find one-armed `if' a bit confusing.
Since return value matters, I suggest to use

  (and (time-less-p nt (current-time)) nt)

>
> (defun org-review-insert-last-review (&optional prompt)
>   "Insert the current date as last review. If prefix argument:
> prompt the user for the date."
>   (interactive "P")
>   (let* ((ts (if prompt
>                 (concat "<" (org-read-date) ">")
>               (format-time-string (car org-time-stamp-formats)))))
>     (save-excursion

I don't think this `save-excursion' is needed.

>       (org-entry-put
>        (if (equal (buffer-name) org-agenda-buffer-name)
>            (or (org-get-at-bol 'org-marker)
>                (org-agenda-error))
>          (point))
>        org-review-last-property-name
>        (cond 

Trailing whitespace.

>         ((equal org-review-timestamp-format 'inactive)

`eq' should be used when comparing symbols.

>          (concat "[" (substring ts 1 -1) "]"))
>         ((equal org-review-timestamp-format 'active)

Ditto.

>          ts)
>         (t (substring ts 1 -1)))))))
>
> (defun org-review-skip ()
>   "Skip entries that are not scheduled to be reviewed."
>   (save-restriction
>     (widen)
>     (let ((next-headline (save-excursion (or (outline-next-heading)
>                                              (point-max)))))
>       (cond
>        ((org-review-toreview-p) nil)
>        (t next-headline)))))

This function doesn't move point (so it skips nothing), is it expected?

Also, I think `save-restriction' + `widen' is only useful for
`outline-next-heading'. And `save-restriction' + `save-excursion' +
`widen' = `org-with-wide-buffer'. You may want to rewrite it to
something like:

  (defun org-review-skip ()
    "Skip entries that are not scheduled to be reviewed."
    (and (not (org-review-toreview-p))
         (org-with-wide-buffer (or (outline-next-heading) (point)))))


Regards,

-- 
Nicolas Goaziou