GNU bug report logs - #23623
Patch to improve function options in find-func.el

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 23623 in the body.
You can then email your comments to 23623 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Robert Weiner <rswgnu <at> gmail.com>
To: bug-gnu-emacs <at> gnu.org, emacs-devel <emacs-devel <at> gnu.org>
Subject: Patch to improve function options in find-func.el
Date: Thu, 26 May 2016 11:52:09 -0400

[Message part 1 (text/plain, inline)]

This patch does three things to find-func.el:
  1. It allows its *-noselect functions to be called with an additional
     option argument of NO-ERROR, to suppress errors when a definition
     is not found and instead simply return nil, allowing them to be
     called with boolean results.

     Although these functions used to return a list of (buffer) in some
     cases of failure, any caller that tested for this result would test
     for a (cdr nil) which is still nil with the new code.  Returning
     a buffer value in cases where the symbol definition was not found
     made no sense anyway since what buffer would that be and what use
     would it be.

  2. Similarly, find-function-do-it returns a boolean value so you can
     tell if it succeeded or failed.

  3. It improves many of the doc strings.

Note that the one large patch section of one function is really just a
change of a line at the end of the function but diff generated a large
context patch for it, maybe the indentation changed.

Please let me know if these changes are acceptable.  They provide many
benefits to potential callers of this package and make the *-noselect
functions more straightforward to use.

-----

In GNU Emacs 25.0.94.1 (x86_64-apple-darwin13.4.0, NS appkit-1265.21
Version 10.9.5 (Build 13F1603))
 of 2016-05-17 built on builder10-9.local
Windowing system distributor 'Apple', version 10.3.1404
Configured using:
 'configure --with-ns '--enable-locallisppath=/Library/Application
 Support/Emacs/${version}/site-lisp:/Library/Application
 Support/Emacs/site-lisp''

Configured features:
NOTIFY ACL LIBXML2 ZLIB TOOLKIT_SCROLL_BARS NS

Important settings:
  value of $LANG: en_US.UTF-8
  locale-coding-system: utf-8-unix

------------

diff -u find-func-orig.el find-func.el
--- find-func-orig.el 2016-05-17 12:16:33.000000000 -0400
+++ find-func.el 2016-05-26 11:14:02.000000000 -0400
@@ -177,9 +177,11 @@
             (setq name rel))))
     (unless (equal name library) name)))

-(defun find-library-name (library)
+(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."
   ;; If the library is byte-compiled, try to find a source library by
   ;; the same name.
   (if (string-match "\\.el\\(c\\(\\..*\\)?\\)\\'" library)
@@ -201,7 +203,8 @@
           (locate-file rel
                        (or find-function-source-path load-path)
                        load-file-rep-suffixes)))))
-   (error "Can't find library %s" library)))
+   (unless no-error
+     (error "Can't find library %s" library))))

 (defvar find-function-C-source-directory
   (let ((dir (expand-file-name "src" source-directory)))
@@ -224,9 +227,11 @@
                ofunc)))
       func))

-(defun find-function-C-source (fun-or-var file type)
+(defun find-function-C-source (fun-or-var file type &optional no-error)
   "Find the source location where FUN-OR-VAR is defined in FILE.
-TYPE should be nil to find a function, or `defvar' to find a variable."
+TYPE should be nil to find a function, or `defvar' to find a variable.
+Signals an error if the source location is not found, unless optional
+NO-ERROR is non-nil, in which case nil is returned."
   (let ((dir (or find-function-C-source-directory
                  (read-directory-name "Emacs C source dir: " nil nil t))))
     (setq file (expand-file-name file dir))
@@ -242,7 +247,7 @@
        (find-function-advised-original fun-or-var)))))
   (with-current-buffer (find-file-noselect file)
     (goto-char (point-min))
-    (unless (re-search-forward
+    (cond ((re-search-forward
      (if type
  (concat "DEFVAR[A-Z_]*[ \t\n]*([ \t\n]*\""
  (regexp-quote (symbol-name fun-or-var))
@@ -251,8 +256,10 @@
        (regexp-quote (subr-name (advice--cd*r fun-or-var)))
        "\""))
      nil t)
-      (error "Can't find source for %s" fun-or-var))
-    (cons (current-buffer) (match-beginning 0))))
+   (cons (current-buffer) (match-beginning 0)))
+  (no-error nil)
+  (t (error "Can't find source for %s" fun-or-var)))))
+

 ;;;###autoload
 (defun find-library (library)
@@ -286,63 +293,66 @@
     (condition-case nil (switch-to-buffer buf) (error (pop-to-buffer
buf)))))

 ;;;###autoload
-(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.
+
+If the definition can't be found and optional NO-ERROR is non-nil,
+return nil; otherwise, signal an error.

 If TYPE is nil, look for a function definition.
 Otherwise, TYPE specifies the kind of definition,
 and it is interpreted via `find-function-regexp-alist'.
 The search is done in the source for library LIBRARY."
   (if (null library)
-      (error "Don't know where `%s' is defined" symbol))
-  ;; Some functions are defined as part of the construct
-  ;; that defines something else.
-  (while (and (symbolp symbol) (get symbol 'definition-name))
-    (setq symbol (get symbol 'definition-name)))
-  (if (string-match "\\`src/\\(.*\\.\\(c\\|m\\)\\)\\'" library)
-      (find-function-C-source symbol (match-string 1 library) type)
-    (when (string-match "\\.el\\(c\\)\\'" library)
-      (setq library (substring library 0 (match-beginning 1))))
-    ;; Strip extension from .emacs.el to make sure symbol is searched in
-    ;; .emacs too.
-    (when (string-match "\\.emacs\\(.el\\)" library)
-      (setq library (substring library 0 (match-beginning 1))))
-    (let* ((filename (find-library-name library))
-   (regexp-symbol (cdr (assq type find-function-regexp-alist))))
-      (with-current-buffer (find-file-noselect filename)
- (let ((regexp (if (functionp regexp-symbol) regexp-symbol
-                        (format (symbol-value regexp-symbol)
-                                ;; Entry for ` (backquote) macro in
loaddefs.el,
-                                ;; (defalias (quote \`)..., has a \ but
-                                ;; (symbol-name symbol) doesn't.  Add an
-                                ;; optional \ to catch this.
-                                (concat "\\\\?"
-                                        (regexp-quote (symbol-name
symbol))))))
-      (case-fold-search))
-  (with-syntax-table emacs-lisp-mode-syntax-table
-    (goto-char (point-min))
-    (if (if (functionp regexp)
-                    (funcall regexp symbol)
-                  (or (re-search-forward regexp nil t)
-                      ;; `regexp' matches definitions using known forms
like
-                      ;; `defun', or `defvar'.  But some
functions/variables
-                      ;; are defined using special macros (or functions),
so
-                      ;; if `regexp' can't find the definition, we look for
-                      ;; something of the form "(SOMETHING <symbol> ...)".
-                      ;; This fails to distinguish function definitions
from
-                      ;; variable declarations (or even uses thereof), but
is
-                      ;; a good pragmatic fallback.
-                      (re-search-forward
-                       (concat "^([^ ]+" find-function-space-re "['(]?"
-                               (regexp-quote (symbol-name symbol))
-                               "\\_>")
-                       nil t)))
- (progn
-  (beginning-of-line)
-  (cons (current-buffer) (point)))
-      (cons (current-buffer) nil))))))))
+      (unless no-error
+ (error "Don't know where `%s' is defined" symbol))
+    ;; Some functions are defined as part of the construct
+    ;; that defines something else.
+    (while (and (symbolp symbol) (get symbol 'definition-name))
+      (setq symbol (get symbol 'definition-name)))
+    (if (string-match "\\`src/\\(.*\\.\\(c\\|m\\)\\)\\'" library)
+ (find-function-C-source symbol (match-string 1 library) type)
+      (when (string-match "\\.el\\(c\\)\\'" library)
+ (setq library (substring library 0 (match-beginning 1))))
+      ;; Strip extension from .emacs.el to make sure symbol is searched in
+      ;; .emacs too.
+      (when (string-match "\\.emacs\\(.el\\)" library)
+ (setq library (substring library 0 (match-beginning 1))))
+      (let* ((filename (find-library-name library))
+     (regexp-symbol (cdr (assq type find-function-regexp-alist))))
+ (with-current-buffer (find-file-noselect filename)
+  (let ((regexp (if (functionp regexp-symbol) regexp-symbol
+  (format (symbol-value regexp-symbol)
+  ;; Entry for ` (backquote) macro in loaddefs.el,
+  ;; (defalias (quote \`)..., has a \ but
+  ;; (symbol-name symbol) doesn't.  Add an
+  ;; optional \ to catch this.
+  (concat "\\\\?"
+  (regexp-quote (symbol-name symbol))))))
+ (case-fold-search))
+    (with-syntax-table emacs-lisp-mode-syntax-table
+      (goto-char (point-min))
+      (if (if (functionp regexp)
+      (funcall regexp symbol)
+    (or (re-search-forward regexp nil t)
+ ;; `regexp' matches definitions using known forms like
+ ;; `defun', or `defvar'.  But some functions/variables
+ ;; are defined using special macros (or functions), so
+ ;; if `regexp' can't find the definition, we look for
+ ;; something of the form "(SOMETHING <symbol> ...)".
+ ;; This fails to distinguish function definitions from
+ ;; variable declarations (or even uses thereof), but is
+ ;; a good pragmatic fallback.
+ (re-search-forward
+ (concat "^([^ ]+" find-function-space-re "['(]?"
+ (regexp-quote (symbol-name symbol))
+ "\\_>")
+ nil t)))
+  (progn
+    (beginning-of-line)
+    (cons (current-buffer) (point)))))))))))

 (defun find-function-library (function &optional lisp-only verbose)
   "Return the pair (ORIG-FUNCTION . LIBRARY) for FUNCTION.
@@ -362,9 +372,9 @@
         aliases)
     ;; 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
                             (concat aliases
                                     (format-message
@@ -386,25 +396,26 @@
            ((symbol-file function 'defun))))))

 ;;;###autoload
-(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.
+Signals an error if FUNCTION is null.

 Finds the source file containing the definition of FUNCTION
 in a buffer and the point of the definition.  The buffer is
-not selected.  If the function definition can't be found in
-the buffer, returns (BUFFER).
+not selected.

-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.

 If the file where FUNCTION is defined is not known, then it is
 searched for in `find-function-source-path' if non-nil, otherwise
 in `load-path'."
   (if (not function)
-    (error "You didn't specify a function"))
+      (error "You didn't specify a function"))
   (let ((func-lib (find-function-library function lisp-only t)))
-    (find-function-search-for-symbol (car func-lib) nil (cdr func-lib))))
+    (find-function-search-for-symbol (car func-lib) nil (cdr func-lib)
+     no-error)))

 (defun find-function-read (&optional type)
   "Read and return an interned symbol, defaulting to the one near point.
@@ -432,7 +443,9 @@
                    t nil nil (and symb (symbol-name symb)))))))

 (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.
+Return t if SYMBOL is found, else nil.
+
 TYPE is nil to search for a function definition,
 or else `defvar' or `defface'.

@@ -454,11 +467,13 @@
       (funcall switch-fn new-buf)
       (when new-point (goto-char new-point))
       (recenter find-function-recenter-line)
-      (run-hooks 'find-function-after-hook))))
+      (run-hooks 'find-function-after-hook)
+      t)))

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

 Finds the source file containing the definition of the function
 near point (selected by `function-called-at-point') in a buffer and
@@ -474,6 +489,7 @@
 ;;;###autoload
 (defun find-function-other-window (function)
   "Find, in another window, the definition of FUNCTION near point.
+Return t if FUNCTION is found, else nil.

 See `find-function' for more details."
   (interactive (find-function-read))
@@ -482,18 +498,21 @@
 ;;;###autoload
 (defun find-function-other-frame (function)
   "Find, in another frame, the definition of FUNCTION near point.
+Return t if FUNCTION is found, else nil.

 See `find-function' for more details."
   (interactive (find-function-read))
   (find-function-do-it function nil 'switch-to-buffer-other-frame))

 ;;;###autoload
-(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.

 Finds the library containing the definition of VARIABLE in a buffer and
 the point of the definition.  The buffer is not selected.
-If the variable's definition can't be found in the buffer, return (BUFFER).
+
+If the definition can't be found and optional NO-ERROR is non-nil,
+return nil; otherwise, signal an error.

 The library where VARIABLE is defined is searched for in FILE or
 `find-function-source-path', if non-nil, otherwise in `load-path'."
@@ -502,11 +521,12 @@
     (let ((library (or file
                        (symbol-file variable 'defvar)
                        (help-C-file-name variable 'var))))
-      (find-function-search-for-symbol variable 'defvar library))))
+      (find-function-search-for-symbol variable 'defvar library
no-error))))

 ;;;###autoload
 (defun find-variable (variable)
   "Find the definition of the VARIABLE at or before point.
+Return t if VARIABLE is found, else nil.

 Finds the library containing the definition of the variable
 near point (selected by `variable-at-point') in a buffer and
@@ -523,6 +543,7 @@
 ;;;###autoload
 (defun find-variable-other-window (variable)
   "Find, in another window, the definition of VARIABLE near point.
+Return t if VARIABLE is found, else nil.

 See `find-variable' for more details."
   (interactive (find-function-read 'defvar))
@@ -531,47 +552,56 @@
 ;;;###autoload
 (defun find-variable-other-frame (variable)
   "Find, in another frame, the definition of VARIABLE near point.
+Return t if VARIABLE is found, else nil.

 See `find-variable' for more details."
   (interactive (find-function-read 'defvar))
   (find-function-do-it variable 'defvar 'switch-to-buffer-other-frame))

 ;;;###autoload
-(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.
+The buffer is not selected.  SYMBOL may be a symbol or a string.
+If the definition can't be found and optional NO-ERROR is non-nil,
+return nil; otherwise, signal an error.
+
 TYPE says what type of definition: nil for a function, `defvar' for a
 variable, `defface' for a face.  This function does not switch to the
 buffer nor display it.

-The library where SYMBOL is defined is searched for in FILE or
+The library where SYMBOL is defined is searched for in optional FILE or
 `find-function-source-path', if non-nil, otherwise in `load-path'."
   (cond
+   ((and (stringp symbol)
+ (setq symbol (intern-soft symbol))
+ ;; Fall through to next type
+ nil))
    ((not symbol)
     (error "You didn't specify a symbol"))
    ((null type)
-    (find-function-noselect symbol))
+    (find-function-noselect symbol nil no-error))
    ((eq type 'defvar)
-    (find-variable-noselect symbol file))
+    (find-variable-noselect symbol file no-error))
    (t
     (let ((library (or file (symbol-file symbol type))))
-      (find-function-search-for-symbol symbol type library)))))
+      (find-function-search-for-symbol symbol type library no-error)))))

 ;; For symmetry, this should be called find-face; but some programs
 ;; assume that, if that name is defined, it means something else.
 ;;;###autoload
 (defun find-face-definition (face)
   "Find the definition of FACE.  FACE defaults to the name near point.
+Return t if FACE is found, else nil.

-Finds the Emacs Lisp library containing the definition of the face
+Find the Emacs Lisp library containing the definition of the face
 near point (selected by `variable-at-point') in a buffer and
-places point before the definition.
+place point before the definition.

 Set mark before moving, if the buffer already existed.

-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
+`find-function-recenter-line' and `find-function-after-hook'."
   (interactive (find-function-read 'defface))
   (find-function-do-it face 'defface 'switch-to-buffer))

[Message part 2 (text/html, inline)]

Message #8 received at 23623 <at> debbugs.gnu.org (full text, mbox):

From: Noam Postavsky <npostavs <at> users.sourceforge.net>
To: Robert Weiner <rswgnu <at> gmail.com>
Cc: 23623 <at> debbugs.gnu.org
Subject: Re: bug#23623: Patch to improve function options in find-func.el
Date: Mon, 06 Nov 2017 20:03:38 -0500

Robert Weiner <rswgnu <at> gmail.com> writes:

> Note that the one large patch section of one function is really just a
> change of a line at the end of the function but diff generated a large
> context patch for it, maybe the indentation changed.

The patch seems to have got mangled by some line wrapping and similar.
Please send as attachment instead.  Please don't post to both
emacs-devel and bug-gnu-emacs.

> Please let me know if these changes are acceptable.  They provide many
> benefits to potential callers of this package and make the *-noselect
> functions more straightforward to use.

I think it's okay for master.

> +Visit the library in a buffer, and return a (buffer . position) pair,

The convention we use in docstrings is that placeholders in structures
should be in upper case.  The way you wrote it here would be describing
a function which does (cons 'buffer 'position).

Message #11 received at 23623 <at> debbugs.gnu.org (full text, mbox):

From: Robert Weiner <rsw <at> gnu.org>
To: Noam Postavsky <npostavs <at> users.sourceforge.net>
Cc: 23623 <at> debbugs.gnu.org
Subject: Re: bug#23623: Patch to improve function options in find-func.el
Date: Sun, 17 Dec 2017 23:33:04 -0500

[Message part 1 (text/plain, inline)]

On Mon, Nov 6, 2017 at 8:03 PM, Noam Postavsky <npostavs <at> users.sourceforge.
net> wrote:

> Robert Weiner <rswgnu <at> gmail.com> writes:
>
>
> I think it's okay for master.
>

​Sorry, I lost track of this and didn't get back to you.​

> ​​
>
> ​​
> > +Visit the library in a buffer, and return a (buffer . position) pair,
> ​​
>
> ​​
> The convention we use in docstrings is that placeholders in structures
> ​​
> should be in upper case.  The way you wrote it here would be describing
> ​​
> a function which does (cons 'buffer 'position).
>
​​
​I have made this requested change and herein attach the patch.  I hope
you can integrate it sometime.

Bob

​

[Message part 2 (text/html, inline)]

[find-func.el.patch (application/octet-stream, attachment)]

Message #14 received at 23623 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: rswgnu <at> gmail.com
Cc: 23623 <at> debbugs.gnu.org, npostavs <at> users.sourceforge.net
Subject: Re: bug#23623: Patch to improve function options in find-func.el
Date: Mon, 18 Dec 2017 18:07:24 +0200

> From: Robert Weiner <rsw <at> gnu.org>
> Date: Sun, 17 Dec 2017 23:33:04 -0500
> Cc: 23623 <at> 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.

Message #17 received at 23623 <at> debbugs.gnu.org (full text, mbox):

From: Robert Weiner <rsw <at> gnu.org>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 23623 <at> debbugs.gnu.org, Noam Postavsky <npostavs <at> users.sourceforge.net>
Subject: Re: bug#23623: Patch to improve function options in find-func.el
Date: Mon, 18 Dec 2017 11:24:44 -0500

[Message part 1 (text/plain, inline)]

On Mon, Dec 18, 2017 at 11:07 AM, Eli Zaretskii <eliz <at> gnu.org> wrote:

> > From: Robert Weiner <rsw <at> gnu.org>
> > Date: Sun, 17 Dec 2017 23:33:04 -0500
> > Cc: 23623 <at> 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:
>

​Thanks for the feedback.  I'll work on it.  I often find
that I have to make the first line of docstrings long
when it involves 3 or more arguments.  Is it preferable
in such cases where the text cannot be shortened enough
to discuss only some of the arguments on the first line
and document the rest afterwards?

Bob

[Message part 2 (text/html, inline)]

Message #20 received at 23623 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: rswgnu <at> gmail.com
Cc: 23623 <at> debbugs.gnu.org, npostavs <at> users.sourceforge.net
Subject: Re: bug#23623: Patch to improve function options in find-func.el
Date: Mon, 18 Dec 2017 18:49:56 +0200

> From: Robert Weiner <rsw <at> gnu.org>
> Date: Mon, 18 Dec 2017 11:24:44 -0500
> Cc: Noam Postavsky <npostavs <at> users.sourceforge.net>, 23623 <at> debbugs.gnu.org
> 
> I often find
> that I have to make the first line of docstrings long
> when it involves 3 or more arguments.  Is it preferable
> in such cases where the text cannot be shortened enough
> to discuss only some of the arguments on the first line
> and document the rest afterwards?

If there's no other way, yes.

A better alternative is to mention all the arguments, but refrain from
describing the function's effect in full, leaving the detailed
description to the rest of the doc string.  IOW, say only the main
part in the first sentence.

Message #23 received at 23623 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Kangas <stefan <at> marxist.se>
To: Robert Weiner <rsw <at> gnu.org>
Cc: Noam Postavsky <npostavs <at> users.sourceforge.net>,
 Eli Zaretskii <eliz <at> gnu.org>, rswgnu <at> gmail.com, 23623 <at> debbugs.gnu.org
Subject: Re: bug#23623: Patch to improve function options in find-func.el
Date: Fri, 08 Nov 2019 05:16:47 +0100

Robert Weiner <rsw <at> gnu.org> writes:

> On Mon, Dec 18, 2017 at 11:07 AM, Eli Zaretskii <eliz <at> gnu.org> wrote:
>
>  > From: Robert Weiner <rsw <at> gnu.org>
>  > Date: Sun, 17 Dec 2017 23:33:04 -0500
>  > Cc: 23623 <at> 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:
>
> ​Thanks for the feedback.  I'll work on it.

That was almost 2 years ago.  What's the current status here?

Best regards,
Stefan Kangas

Message #26 received at 23623 <at> debbugs.gnu.org (full text, mbox):

From: Lars Ingebrigtsen <larsi <at> gnus.org>
To: Stefan Kangas <stefan <at> marxist.se>
Cc: rswgnu <at> gmail.com, Eli Zaretskii <eliz <at> gnu.org>,
 Noam Postavsky <npostavs <at> users.sourceforge.net>, 23623 <at> debbugs.gnu.org,
 Robert Weiner <rsw <at> gnu.org>
Subject: Re: bug#23623: Patch to improve function options in find-func.el
Date: Tue, 11 Aug 2020 17:25:27 +0200

Stefan Kangas <stefan <at> marxist.se> writes:

>> ​Thanks for the feedback.  I'll work on it.
>
> That was almost 2 years ago.  What's the current status here?

And that was more than half a month ago.

The patch no longer applies to Emacs 28 -- there's been many changes to
find-func in the meantime.

Eli had a lot of comments on the patch, mostly stylistic, but also about
how this change would be somewhat non-backwards compatible.  So while I
think this sounds like a generally useful change, it seems pretty
invasive and extensive uncertain gain, so I'm closing this bug report.

If somebody wants to do further work here, please respond and we'll
reopen.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Message #33 received at 23623 <at> debbugs.gnu.org (full text, mbox):

From: Lars Ingebrigtsen <larsi <at> gnus.org>
To: Stefan Kangas <stefan <at> marxist.se>
Cc: rswgnu <at> gmail.com, 23623 <at> debbugs.gnu.org,
 Noam Postavsky <npostavs <at> users.sourceforge.net>, Robert Weiner <rsw <at> gnu.org>
Subject: Re: bug#23623: Patch to improve function options in find-func.el
Date: Tue, 11 Aug 2020 17:36:39 +0200

Lars Ingebrigtsen <larsi <at> gnus.org> writes:

> Stefan Kangas <stefan <at> marxist.se> writes:
>
>>> ​Thanks for the feedback.  I'll work on it.
>>
>> That was almost 2 years ago.  What's the current status here?
>
> And that was more than half a month ago.

(I mean half a year ago.)

> Eli had a lot of comments on the patch, mostly stylistic, but also about
> how this change would be somewhat non-backwards compatible.  So while I
> think this sounds like a generally useful change, it seems pretty
> invasive and extensive uncertain gain, so I'm closing this bug report.

That's...  some word soup, but perhaps people understood it anyway.  :-/

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.