bug#23623: Patch to improve function options in find-func.el

[Eli Zaretskii]

> From: Robert Weiner <rsw@gnu.org>
> Date: Sun, 17 Dec 2017 23:33:04 -0500
> Cc: 23623@debbugs.gnu.org
> 
> ​​​I have made this requested change and herein attach the patch.  I hope
> you can integrate it sometime.

Thanks.  I have a few minor comments:

First, please provide a ChangeLog-style commit log for the changes
(see CONTRIBUTE for the details).

> +(defun find-library-name (library &optional no-error)
>    "Return the absolute file name of the Emacs Lisp source of LIBRARY.
> -LIBRARY should be a string (the name of the library)."
> +LIBRARY should be a string (the name of the library).
> +Signals an error if the source location is not found, unless optional
> +NO-ERROR is non-nil, in which case nil is returned."

Please try to avoid using passive tense in documentation and comments,
doing so makes the text longer and more complex.  In this case:

 Signal an error if the source location is not found, unless optional
 NO-ERROR is non-nil, in which case silently return nil.

(Note that I also modified the tense of the verbs to be consistent
with the first sentence of the doc string.)

Similar issues exist with other doc string changes in this patch.

> -(defun find-function-search-for-symbol (symbol type library)
> -  "Search for SYMBOL's definition of type TYPE in LIBRARY.
> -Visit the library in a buffer, and return a cons cell (BUFFER . POSITION),
> -or just (BUFFER . nil) if the definition can't be found in the file.
> +(defun find-function-search-for-symbol (symbol type library &optional 
> no-error)
> +  "Search for SYMBOL's definition of TYPE in LIBRARY.
> +Visit the library in a buffer, and return a (BUFFER . POSITION) pair,
> +or nil if the definition can't be found in the library.

This second alternative of the return value makes this an incompatible
change.  Is that really necessary?  It also makes it impossible to
distinguish between the two kinds of failures.

>      ;; FIXME for completeness, it might be nice to print something like:
>      ;; foo (which is advised), which is an alias for bar (which is advised).
> -    (while (and def (symbolp def))
> -      (or (eq def function)
> -          (not verbose)
> +    ;; 5/26/2016 - fixed to not loop forever when (eq def function)
> +    (while (and def (symbolp def) (not (eq def function)))
> +      (or (not verbose)
>            (setq aliases (if aliases

The above seems to be an unrelated change.  Also, please don't leave
dates of changes in the sources (or maybe the whole comment is
unnecessary).

> -(defun find-function-noselect (function &optional lisp-only)
> -  "Return a pair (BUFFER . POINT) pointing to the definition of FUNCTION.
> +(defun find-function-noselect (function &optional lisp-only no-error)
> +  "Return a (BUFFER . POINT) pair pointing to the definition of FUNCTION or 
> nil if not found.

The first sentence is too long, it should fit on the default window
width of 80 columns, and preferably be even shorter.

> +Signals an error if FUNCTION is null.
   ^^^^^^^
"Signal"

> -If FUNCTION is a built-in function, this function normally
> -attempts to find it in the Emacs C sources; however, if LISP-ONLY
> -is non-nil, signal an error instead.
> +Built-in functions are found within Emacs C sources unless
> +optional LISP-ONLY is non-nil, in which case an error is signaled
> +unless optional NO-ERROR is non-nil.

Here you took text that was very clear and modified it to use passive
tense, which made it less so.  Most of the changes are unnecessary
anyway, as you just needed to add what happens with NO-ERROR non-nil.
So I'd use something like this:

  If FUNCTION is a built-in function, this function normally
  attempts to find it in the Emacs C sources; however, if LISP-ONLY
  is non-nil, it signals an error instead.  If the optional argument
  NO-ERROR is non-nil, it returns nil instead of signaling an error.

>    (if (not function)
> -    (error "You didn't specify a function"))
> +      (error "You didn't specify a function"))

Hmm... why did the indentation change here?

>  (defun find-function-do-it (symbol type switch-fn)
> -  "Find Emacs Lisp SYMBOL in a buffer and display it.
> +  "Find Emacs Lisp SYMBOL of TYPE in a buffer, display it with SWITCH-FN and 
> return t, else nil if not found.

Once again, this sentence is too long for the first sentence of a doc
string.

I also question the decision to return t if the function succeeds:
couldn't it return a more useful value, like the buffer where the
function is displayed?

>  (defun find-function (function)
>    "Find the definition of the FUNCTION near point.
> +Return t if FUNCTION is found, else nil.

Likewise here (and elsewhere in a few similar functions).

> -(defun find-variable-noselect (variable &optional file)
> -  "Return a pair `(BUFFER . POINT)' pointing to the definition of VARIABLE.
> +(defun find-variable-noselect (variable &optional file no-error)
> +  "Return a (BUFFER . POINT) pair pointing to the definition of VARIABLE or 
> nil if not found.

Sentence too long.

> -(defun find-definition-noselect (symbol type &optional file)
> -  "Return a pair `(BUFFER . POINT)' pointing to the definition of SYMBOL.
> -If the definition can't be found in the buffer, return (BUFFER).
> +(defun find-definition-noselect (symbol type &optional file no-error)
> +  "Return a (BUFFER . POINT) pair pointing to the definition of SYMBOL or 
> nil if not found.

Likewise.

> -The library where FACE is defined is searched for in
> -`find-function-source-path', if non-nil, otherwise in `load-path'.
> -See also `find-function-recenter-line' and `find-function-after-hook'."
> +The library searched for FACE is given by `find-function-source-path',
> +if non-nil, otherwise `load-path'.  See also

I agree that the original text was sub-optimal, but saying that a
library is "given by" a path variable is IMO confusing.  How about
this variant instead:

  The library that defines FACE is looked for in directories specified
  by `find-function-source-path', if that is non-nil, or `load-path'
  otherwise.

Thanks again for working on this.