From mboxrd@z Thu Jan 1 00:00:00 1970 From: Nicolas Goaziou Subject: Re: org-review-schedule Date: Fri, 25 Apr 2014 08:51:39 +0200 Message-ID: <87fvl1lx50.fsf@gmail.com> References: <877g6leltk.fsf@bzg.ath.cx> Mime-Version: 1.0 Content-Type: text/plain Return-path: Received: from eggs.gnu.org ([2001:4830:134:3::10]:48992) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WdZyf-0000Dx-3J for emacs-orgmode@gnu.org; Fri, 25 Apr 2014 02:51:22 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1WdZyZ-00065v-B3 for emacs-orgmode@gnu.org; Fri, 25 Apr 2014 02:51:17 -0400 In-Reply-To: (Alan Schmitt's message of "Thu, 24 Apr 2014 13:51:02 +0200") List-Id: "General discussions about Org-mode." List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: emacs-orgmode-bounces+geo-emacs-orgmode=m.gmane.org@gnu.org Sender: emacs-orgmode-bounces+geo-emacs-orgmode=m.gmane.org@gnu.org To: Alan Schmitt Cc: Bastien , emacs-orgmode Hello, Alan Schmitt 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