Re: [O] inherit priority

[Nicolas Goaziou]

Hello,

Jesse Johnson <address@hidden> writes:

> Since you want to comment I guess you want the patch in the e-mail
> body rather than attached. Here goes nothing.

Thank you!

It looks good. Some minor comments follow.

> From bb02cd6c00b32155c0a25f409f1bfa4160b2ddcd Mon Sep 17 00:00:00 2001
> From: Jesse Johnson <address@hidden>
> Date: Sun, 22 Apr 2018 18:12:54 -0700
> Subject: [PATCH] Add priority inheritance

You need to describe here what functions or variables changed here,
e.g.,

  * lisp/org-agenda (org-search-view): ...

>  (defcustom org-get-priority-function nil
> -  "Function to extract the priority from a string.
> -The string is normally the headline.  If this is nil Org computes the
> -priority from the priority cookie like [#A] in the headline.  It returns
> -an integer, increasing by 1000 for each priority level.
> -The user can set a different function here, which should take a string
> -as an argument and return the numeric priority."
> +  "Function to extract the priority from current line.
> +The line is always a headline.
> +
> +If this is nil Org computes the priority of the headline from a
> +priority cookie like [#A]. It returns an integer, increasing by

You need to add two spaces after a full stop.

> +1000 for each priority level (see
> +`org-priority-char-to-integer').

> +(defcustom org-use-priority-inheritance nil
> +  "Whether headline priority is inherited from its parents.

"Non-nil means headline priority is..."

> +If non-nil then the first explicit priority found when searching
> +up the headline tree applies.  Thus a child headline can override
> +its parent's priority.
> +
> +When nil, explicit priorities only apply to the headline they are
> +given on.
> +
> +Regardless of setting, if no explicit priority is found then the
> +default priority is used."
> +  :group 'org-priorities
> +  :type 'boolean)

You need to add the following keywords: 

  :package-version '(Org . "9.3")

and possibly 

  :safe t

> +(defun org-priority-char-to-integer (character)
> +  "Convert priority CHARACTER to an integer priority."
> +  (* 1000 (- org-lowest-priority character)))
> +
> +(defun org-priority-integer-to-char (integer)
> +  "Convert priority INTEGER to a character priority."
> +  (- org-lowest-priority (/ integer 1000)))

I think those can be internal functions, so they should be renamed
`org--priority-char-to-integer' and `org--priority-integer-to-char'.

> +(defun org-get-priority (&optional pos local)
> +  "Get integer priority at POS.
> +POS defaults to point.  If LOCAL is non-nil priority inheritance
> +is ignored regardless of the value of
> +`org-use-priority-inheritance'.  Returns nil if no priority can be

Return nil if...

> +determined at POS."
> +  (save-excursion
> +    (save-restriction
> +      (widen)
> +      (goto-char (or pos (point)))

`save-excursion' + `save-restriction' + `widen' + `goto-char' = 
`org-with-point-at'

So the above would be:

  (org-with-point-at pos
    ...)

> +      (beginning-of-line)
> +      (if (not (looking-at org-heading-regexp))
> +      (return nil)

Indentation looks wrong, but it should be:

  (unless (looking-at org-heading-regexp)
    ...)

> +    (save-match-data
> +      (cl-loop
> +       (if (functionp org-get-priority-function)
> +           (let ((priority (funcall org-get-priority-function)))
> +         (unless (eq priority t)
> +           (return priority)))
> +         (when (looking-at org-priority-regexp)
> +           (return (org-priority-char-to-integer
> +            (string-to-char (match-string-no-properties 2))))))
> +       (unless (and (not local)
> +            org-use-priority-inheritance
> +            (org-up-heading-safe))
> +         (return (org-priority-char-to-integer
> org-default-priority)))))))))

You can write a simpler function. Please have a look at `org-get-tags'
and use `org-complex-heading-regexp' to get priority cookie.

Also could you throw in a bunch of tests in "contrib/lisp/test-org.el"
and update the manual accordingly?

Regards,

-- 
Nicolas Goaziou