Re: master 49192e9: Strip "(fn...)" from output of `describe-mode' (bug#38222)

Re: master 49192e9: Strip "(fn...)" from output of `describe-mode' (bug#38222)

[Stefan Monnier]

Hola Juanma,

>     * lisp/help.el (help--doc-without-fn): New function.

I think you're looking for `help-split-fundoc`.


        Stefan

Re: master 49192e9: Strip "(fn...)" from output of `describe-mode' (bug#38222)

[Juanma Barranquero]

[-- Attachment #1: Type: text/plain, Size: 253 bytes --]

help-split-fundoc returns nil if the docstring has no usage string.

(help-split-fundoc (documentation 'auto-composition-mode)
'auto-composition-mode) => nil

So it's either my function, or adding optional args to help-split-fundoc.
What do you prefer?

[-- Attachment #2: Type: text/html, Size: 338 bytes --]

Re: master 49192e9: Strip "(fn...)" from output of `describe-mode' (bug#38222)

[Stefan Monnier]

> help-split-fundoc returns nil if the docstring has no usage string.
>
> (help-split-fundoc (documentation 'auto-composition-mode)
> 'auto-composition-mode) => nil

IIRC you can use

    (let* ((doc (documentation NAME))
           (fd (help-split-fundoc doc NAME)))
      (if fd (cdr fd) doc))

But I agree that `help-split-fundoc` is inconvenient.
Improvements welcome.  Maybe it should always return a pair (USAGE
. DOC) so we could just do:

    (cdr (help-split-fundoc (documentation NAME) NAME)


-- Stefan

Re: master 49192e9: Strip "(fn...)" from output of `describe-mode' (bug#38222)

[Juanma Barranquero]

[-- Attachment #1: Type: text/plain, Size: 687 bytes --]

On Tue, Nov 19, 2019 at 8:23 PM Stefan Monnier <monnier@iro.umontreal.ca>
wrote:

> IIRC you can use
>
>     (let* ((doc (documentation NAME))
>            (fd (help-split-fundoc doc NAME)))
>       (if fd (cdr fd) doc))

Obviously, but at that point it's not cleaner (or clearer) than simply
calling a function that does replace-regexp-in-string, IMO.

> Maybe it should always return a pair (USAGE
> . DOC) so we could just do:
>
>     (cdr (help-split-fundoc (documentation NAME) NAME)

I'm not against it, but that's changing the behavior of a non-internal
function (it's already used like ten times just in our sources) which has
been like that for at least six years, likely more.

[-- Attachment #2: Type: text/html, Size: 917 bytes --]

Re: master 49192e9: Strip "(fn...)" from output of `describe-mode' (bug#38222)

[Stefan Monnier]

>> IIRC you can use
>>
>>     (let* ((doc (documentation NAME))
>>            (fd (help-split-fundoc doc NAME)))
>>       (if fd (cdr fd) doc))
>
> Obviously, but at that point it's not cleaner (or clearer) than simply
> calling a function that does replace-regexp-in-string, IMO.

It is because it hides the particular detail about the format we use.

>> Maybe it should always return a pair (USAGE
>> . DOC) so we could just do:
>>
>>     (cdr (help-split-fundoc (documentation NAME) NAME)
>
> I'm not against it, but that's changing the behavior of a non-internal
> function (it's already used like ten times just in our sources) which has
> been like that for at least six years, likely more.

Indeed, to do that we need to see how it's used to see whether it can
safely be changed as-is or whether it needs to depend on some new arg
or something.


        Stefan

Re: master 49192e9: Strip "(fn...)" from output of `describe-mode' (bug#38222)

[Juanma Barranquero]

[-- Attachment #1: Type: text/plain, Size: 732 bytes --]

On Tue, Nov 19, 2019 at 9:05 PM Stefan Monnier <monnier@iro.umontreal.ca>
wrote:

> It is because it hides the particular detail about the format we use.

As the function I added it's internal and just used from describe-mode, I
don't think leaking that detail is something to worry about. In fact, you
can un-leak it just by renaming the function to
help--docstring-without-usage ;-)

> Indeed, to do that we need to see how it's used to see whether it can
> safely be changed as-is or whether it needs to depend on some new arg
> or something.

Of course. Easy to do for our uses of help-split-fundoc, and I'm willing to
do it. But we don't know if and how it is used elsewhere. There are a few
uses already in ELPA, for example.

[-- Attachment #2: Type: text/html, Size: 1310 bytes --]

Re: master 49192e9: Strip "(fn...)" from output of `describe-mode' (bug#38222)

[Stefan Monnier]

>> Indeed, to do that we need to see how it's used to see whether it can
>> safely be changed as-is or whether it needs to depend on some new arg
>> or something.
> Of course. Easy to do for our uses of help-split-fundoc, and I'm willing to
> do it. But we don't know if and how it is used elsewhere. There are a few
> uses already in ELPA, for example.

What I'm suggesting is to look at all the usage we can find and see if
such a change would break that code.  If none is broken, there's a high
probability that other code we don't know about wouldn't be
broken either.


        Stefan

RE: master 49192e9: Strip "(fn...)" from output of `describe-mode' (bug#38222)

[Drew Adams]

> What I'm suggesting is to look at all the usage we can find and see if
> such a change would break that code.  If none is broken, there's a high
> probability that other code we don't know about wouldn't be
> broken either.

FWIW, I have 6 occurrences of `help-split-fundoc' in my library
`help-fns+.el'.  But those occurrences are derived from the
`help-fns.el' code.

FWIW2, I've already updated `help-fns+.el' to use code similar
to what Juanma proposed, for `describe-mode'.  But I can no
doubt adapt both that and the `help-split-fundoc' occurrences
to whatever changes you end up making.

https://www.emacswiki.org/emacs/download/help-fns%2b.el

Re: master 49192e9: Strip "(fn...)" from output of `describe-mode' (bug#38222)

[Juanma Barranquero]

[-- Attachment #1: Type: text/plain, Size: 1349 bytes --]

On Tue, Nov 19, 2019 at 9:56 PM Stefan Monnier <monnier@iro.umontreal.ca>
wrote:

> What I'm suggesting is to look at all the usage we can find and see if
> such a change would break that code.  If none is broken, there's a high
> probability that other code we don't know about wouldn't be
> broken either.

After looking at all the code calling help-split-fundoc in our sources (not
ELPA), I'm unconvinced that changing it to return (nil . DOC) when there's
no usage would be a good idea.

Basically, all callers do the moral equivalent of

(let ((ud (help-split-fundoc documentation symbol)))
   (if ud
       ;;; (car ud) or (cdr ud)

and assume that (car ud) and (cdr ud) are strings.

For example, take a look at this code from
semantic-grammar-eldoc-get-macro-docstring:

    (let* ((doc (help-split-fundoc (documentation expander t) expander)))
      (cond
       (doc
        (setq doc (car doc))
        (string-match "\\`[^ )]* ?" doc)
        (setq doc (concat "(" (substring doc (match-end 0)))))

which would set doc to nil and then try to string-match it.

There are ten cases like this just in our sources; each one would require
some bit of tweaking, however small.

A far less intrusive option IMHO would be to add an optional argument
SECTION, with SECTION = nil as it is now, and SECTION 'usage or 'doc
returning only that part.

[-- Attachment #2: Type: text/html, Size: 1948 bytes --]

Re: master 49192e9: Strip "(fn...)" from output of `describe-mode' (bug#38222)

[Stefan Monnier]

> For example, take a look at this code from
> semantic-grammar-eldoc-get-macro-docstring:
>
>     (let* ((doc (help-split-fundoc (documentation expander t) expander)))
>       (cond
>        (doc
>         (setq doc (car doc))
>         (string-match "\\`[^ )]* ?" doc)
>         (setq doc (concat "(" (substring doc (match-end 0)))))

So it's just not an option, thanks for looking at it.

> A far less intrusive option IMHO would be to add an optional argument
> SECTION, with SECTION = nil as it is now, and SECTION 'usage or 'doc
> returning only that part.

And if it's just t it could always return a pair.
Sounds good.


        Stefan

Re: master 49192e9: Strip "(fn...)" from output of `describe-mode' (bug#38222)

[Juanma Barranquero]

[-- Attachment #1.1: Type: text/plain, Size: 1181 bytes --]

On Fri, Nov 22, 2019 at 10:45 PM Stefan Monnier <monnier@iro.umontreal.ca>
wrote:

> And if it's just t it could always return a pair.
> Sounds good.

Please take a look.

The patches do not include info changes, because help-split-fundoc is not
documented. Should this change, once accepted, be mentioned in NEWS?

BTW, other than the discussed new feature, I've added the following tiny,
sort of incompatible change:

 Before:
- DEF = non-symbol => usage = "(anonymous ARGS...)"
- DEF = symbol s => usage = "(s ARGS...)"

Now:
- DEF = non-symbol OR nil => "(anonymous ARGS...)"
- DEF = other symbol s => "(s ARGS...)"

When callers are not interested in the usage they can already pass a
non-symbol, like "" or 0, but perusing the sources, 3 out of 14 uses of
help-split-fundoc pass nil, and none of them passes a non-symbol.

Treating nil like "don't care" saves two function calls over the current
code (which passes nil through help--docstring-quote), and DEF is
documented as "the function whose usage we're looking for", so
nil = "don't care" makes more sense than "" or 0.

I can revert this part of the patch (is a one-liner), if you're worried
about the incompatibility.

[-- Attachment #1.2: Type: text/html, Size: 2158 bytes --]

[-- Attachment #2: 0001-Make-help-split-fundoc-more-flexible-about-what-retu.patch --]
[-- Type: application/octet-stream, Size: 6123 bytes --]

From 0351f8c2b61624eb9952957dd54655e533cfe573 Mon Sep 17 00:00:00 2001
From: Juanma Barranquero <lekktu@gmail.com>
Date: Sat, 23 Nov 2019 21:33:32 +0100
Subject: [PATCH 1/2] Make help-split-fundoc more flexible about what returns

* lisp/help.el (help-split-fundoc): New arg SECTION to return
only the usage or doc parts of the docstring, or both even if
there is no usage.

* test/lisp/help-tests.el: New file.
---
 lisp/help.el            | 44 ++++++++++++++++++++------------
 test/lisp/help-tests.el | 56 +++++++++++++++++++++++++++++++++++++++++
 2 files changed, 84 insertions(+), 16 deletions(-)
 create mode 100644 test/lisp/help-tests.el

diff --git a/lisp/help.el b/lisp/help.el
index 22f35df1de..06264ae2f3 100644
--- a/lisp/help.el
+++ b/lisp/help.el
@@ -1380,27 +1380,39 @@ help--docstring-quote
 ;; But for various reasons, they are more widely needed, so they were
 ;; moved to this file, which is preloaded.  https://debbugs.gnu.org/17001
 
-(defun help-split-fundoc (docstring def)
+(defun help-split-fundoc (docstring def &optional section)
   "Split a function DOCSTRING into the actual doc and the usage info.
-Return (USAGE . DOC) or nil if there's no usage info, where USAGE info
-is a string describing the argument list of DEF, such as
-\"(apply FUNCTION &rest ARGUMENTS)\".
-DEF is the function whose usage we're looking for in DOCSTRING."
+Return (USAGE . DOC), where USAGE is a string describing the argument
+list of DEF, such as \"(apply FUNCTION &rest ARGUMENTS)\".
+DEF is the function whose usage we're looking for in DOCSTRING.
+With SECTION nil, return nil if there is no usage info; conversely,
+SECTION t means to return (USAGE . DOC) even if there's no usage info.
+When SECTION is \\='usage or \\='doc, return only that part."
   ;; Functions can get the calling sequence at the end of the doc string.
   ;; In cases where `function' has been fset to a subr we can't search for
   ;; function's name in the doc string so we use `fn' as the anonymous
   ;; function name instead.
-  (when (and docstring (string-match "\n\n(fn\\(\\( .*\\)?)\\)\\'" docstring))
-    (let ((doc (unless (zerop (match-beginning 0))
-		 (substring docstring 0 (match-beginning 0))))
-	  (usage-tail (match-string 1 docstring)))
-      (cons (format "(%s%s"
-		    ;; Replace `fn' with the actual function name.
-		    (if (symbolp def)
-			(help--docstring-quote (format "%S" def))
-		      'anonymous)
-		    usage-tail)
-	    doc))))
+  (let* ((found (and docstring
+                     (string-match "\n\n(fn\\(\\( .*\\)?)\\)\\'" docstring)))
+         (doc (if found
+                  (and (memq section '(t nil doc))
+                       (not (zerop (match-beginning 0)))
+                       (substring docstring 0 (match-beginning 0)))
+                docstring))
+         (usage (and found
+                     (memq section '(t nil usage))
+                     (let ((tail (match-string 1 docstring)))
+                       (format "(%s%s"
+                               ;; Replace `fn' with the actual function name.
+                               (if (and (symbolp def) def)
+                                   (help--docstring-quote (format "%S" def))
+                                 'anonymous)
+                               tail)))))
+    (pcase section
+      (`nil (and usage (cons usage doc)))
+      (`t (cons usage doc))
+      (`usage usage)
+      (`doc doc))))
 
 (defun help-add-fundoc-usage (docstring arglist)
   "Add the usage info to DOCSTRING.
diff --git a/test/lisp/help-tests.el b/test/lisp/help-tests.el
new file mode 100644
index 0000000000..28dd8830e4
--- /dev/null
+++ b/test/lisp/help-tests.el
@@ -0,0 +1,56 @@
+;;; help-tests.el --- Tests for help.el  -*- lexical-binding: t; -*-
+
+;; Copyright (C) 2019 Free Software Foundation, Inc.
+
+;; Author: Juanma Barranquero <lekktu@gmail.com>
+;; Keywords: help, internal
+
+;; This file is part of GNU Emacs.
+
+;; GNU Emacs is free software: you can redistribute it and/or modify
+;; it under the terms of the GNU General Public License as published by
+;; the Free Software Foundation, either version 3 of the License, or
+;; (at your option) any later version.
+
+;; GNU Emacs is distributed in the hope that it will be useful,
+;; but WITHOUT ANY WARRANTY; without even the implied warranty of
+;; MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+;; GNU General Public License for more details.
+
+;; You should have received a copy of the GNU General Public License
+;; along with GNU Emacs.  If not, see <https://www.gnu.org/licenses/>.
+
+;;; Code:
+
+(require 'ert)
+
+(ert-deftest help-split-fundoc-SECTION ()
+  "Test new optional arg SECTION."
+  (let* ((doc "Doc first line.\nDoc second line.")
+         (usg "\n\n(fn ARG1 &optional ARG2)")
+         (full (concat doc usg))
+         (usage "(t ARG1 &optional ARG2)"))
+    ;; Docstring has both usage and doc
+    (should (equal (help-split-fundoc full t nil)    `(,usage . ,doc)))
+    (should (equal (help-split-fundoc full t t)      `(,usage . ,doc)))
+    (should (equal (help-split-fundoc full t 'usage)  usage))
+    (should (equal (help-split-fundoc full t 'doc)    doc))
+    ;; Docstring has no usage, only doc
+    (should (equal (help-split-fundoc doc t nil)      nil))
+    (should (equal (help-split-fundoc doc t t)       `(nil . ,doc)))
+    (should (equal (help-split-fundoc doc t 'usage)  nil))
+    (should (equal (help-split-fundoc doc t 'doc)    doc))
+    ;; Docstring is only usage, no doc
+    (should (equal (help-split-fundoc usg t nil)     `(,usage . nil)))
+    (should (equal (help-split-fundoc usg t t)       `(,usage . nil)))
+    (should (equal (help-split-fundoc usg t 'usage)  usage))
+    (should (equal (help-split-fundoc usg t 'doc)    nil))
+    ;; Docstring is null
+    (should (equal (help-split-fundoc nil t nil)     nil))
+    (should (equal (help-split-fundoc nil t t)       '(nil)))
+    (should (equal (help-split-fundoc nil t 'usage)  nil))
+    (should (equal (help-split-fundoc nil t 'doc)    nil))))
+
+(provide 'help-tests)
+
+;;; help-tests.el ends here
-- 
2.23.0.windows.1


[-- Attachment #3: 0002-Rework-previous-change-to-fix-bug-38222.patch --]
[-- Type: application/octet-stream, Size: 1696 bytes --]

From 19e2c488062fb81645f90e791da227f79be45611 Mon Sep 17 00:00:00 2001
From: Juanma Barranquero <lekktu@gmail.com>
Date: Sat, 23 Nov 2019 21:46:03 +0100
Subject: [PATCH 2/2] Rework previous change to fix bug#38222

* lisp/help.el (help--doc-without-fn): Remove.
(describe-mode): Use help-split-fundoc instead.
---
 lisp/help.el | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/lisp/help.el b/lisp/help.el
index 06264ae2f3..c4402ece4e 100644
--- a/lisp/help.el
+++ b/lisp/help.el
@@ -878,10 +878,6 @@ describe-key
             (princ ", which is ")
 	    (describe-function-1 defn)))))))
 \f
-(defun help--doc-without-fn (mode)
-  ;; Remove the (fn...) thingy at the end of the docstring
-  (replace-regexp-in-string "\n\n(fn[^)]*?)\\'" "" (documentation mode)))
-
 (defun describe-mode (&optional buffer)
   "Display documentation of current major mode and minor modes.
 A brief summary of the minor modes comes first, followed by the
@@ -955,7 +951,8 @@ describe-mode
 				     "no indicator"
 				   (format "indicator%s"
 					   indicator))))
-		  (princ (help--doc-without-fn mode-function)))
+		  (princ (help-split-fundoc (documentation mode-function)
+                                            nil 'doc)))
 		(insert-button pretty-minor-mode
 			       'action (car help-button-cache)
 			       'follow-link t
@@ -985,7 +982,7 @@ describe-mode
                                     nil t)
 		(help-xref-button 1 'help-function-def mode file-name)))))
 	(princ ":\n")
-	(princ (help--doc-without-fn major-mode)))))
+	(princ (help-split-fundoc (documentation major-mode) nil 'doc)))))
   ;; For the sake of IELM and maybe others
   nil)
 
-- 
2.23.0.windows.1

Re: master 49192e9: Strip "(fn...)" from output of `describe-mode' (bug#38222)

[Stefan Monnier]

> Please take a look.

Looks good to me, thank you,

> I can revert this part of the patch (is a one-liner), if you're worried
> about the incompatibility.

No, it's much more likely to fix bugs than to introduce ones,


        Stefan