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

classic Classic list List threaded Threaded
12 messages Options
Reply | Threaded
Open this post in threaded view
|

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


Reply | Threaded
Open this post in threaded view
|

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

Juanma Barranquero
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?
Reply | Threaded
Open this post in threaded view
|

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


Reply | Threaded
Open this post in threaded view
|

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

Juanma Barranquero
On Tue, Nov 19, 2019 at 8:23 PM Stefan Monnier <[hidden email]> 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.

Reply | Threaded
Open this post in threaded view
|

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


Reply | Threaded
Open this post in threaded view
|

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

Juanma Barranquero

On Tue, Nov 19, 2019 at 9:05 PM Stefan Monnier <[hidden email]> 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.

Reply | Threaded
Open this post in threaded view
|

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


Reply | Threaded
Open this post in threaded view
|

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

Reply | Threaded
Open this post in threaded view
|

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

Juanma Barranquero
In reply to this post by Stefan Monnier


On Tue, Nov 19, 2019 at 9:56 PM Stefan Monnier <[hidden email]> 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.

Reply | Threaded
Open this post in threaded view
|

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


Reply | Threaded
Open this post in threaded view
|

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

Juanma Barranquero
On Fri, Nov 22, 2019 at 10:45 PM Stefan Monnier <[hidden email]> 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.



0001-Make-help-split-fundoc-more-flexible-about-what-retu.patch (8K) Download Attachment
0002-Rework-previous-change-to-fix-bug-38222.patch (2K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

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