Filling the docstring generated by define-minor-mode

Filling the docstring generated by define-minor-mode

[Juanma Barranquero]

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

Currently, in some cases, define-minor-mode generates parts of a minor
mode's doctring, by using easy-mmode--arg-docstring:

  (defconst easy-mmode--arg-docstring
    "

  If called interactively, enable %s if ARG is positive, and
  disable it if ARG is zero or negative.  If called from Lisp,
  also enable the mode if ARG is omitted or nil, and toggle it
  if ARG is `toggle'; disable the mode otherwise.")

and then adding other references to the minor mode's function or variable.

Minor mode's names are often long and the result is quite ugly. An
easy fix is simply filling the docstring afterwards:

diff --git i/lisp/emacs-lisp/easy-mmode.el w/lisp/emacs-lisp/easy-mmode.el
index be531aab84..23f85a3f4f 100644
--- i/lisp/emacs-lisp/easy-mmode.el
+++ w/lisp/emacs-lisp/easy-mmode.el
@@ -96,9 +96,13 @@ easy-mmode--mode-docstring
     (if (string-match-p "\\bARG\\b" doc)
         doc
-      (let ((argdoc (format easy-mmode--arg-docstring
+      (let ((fill-prefix nil) (fill-column 65)
+            (argdoc (format easy-mmode--arg-docstring
                             mode-pretty-name)))
-        (replace-regexp-in-string "\\(\n\n\\|\\'\\)\\(.\\|\n\\)*\\'"
-                                  (concat argdoc "\\1")
-                                  doc nil nil 1)))))
+        (with-temp-buffer
+          (insert (replace-regexp-in-string "\\(\n\n\\|\\'\\)\\(.\\|\n\\)*\\'"
+                                            (concat argdoc "\\1")
+                                            doc nil nil 1))
+          (fill-region (point-min) (point-max) 'left t)
+          (buffer-string))))))

 ;;;###autoload


In the patch above I've used 65 as fill-column because I think it
gives better results for most minor mode's names than the suggested
60. Other values are of course posible, and can be discussed later if
this is deemed useful.

The following images show the docstring for
`display-fill-column-indicator-mode', as it is right now, and after
auto-filling it to 65 columns.

WDYT?

[-- Attachment #2: before.png --]
[-- Type: image/png, Size: 28777 bytes --]

[-- Attachment #3: after.png --]
[-- Type: image/png, Size: 27491 bytes --]

Re: Filling the docstring generated by define-minor-mode

[Basil L. Contovounesios]

Juanma Barranquero <lekktu@gmail.com> writes:

[...]

> In the patch above I've used 65 as fill-column because I think it
> gives better results for most minor mode's names than the suggested
> 60. Other values are of course posible, and can be discussed later if
> this is deemed useful.

[...]

> WDYT?

FWIW, I find this useful.  My only questions are a) whether we'd want to
introduce optional docstring filling more systematically, e.g. during
*Help* buffer generation, and b) whether the fill-column can be
controlled by emacs-lisp-docstring-fill-column or some new user option
along the same lines.

Thanks,

-- 
Basil

Re: Filling the docstring generated by define-minor-mode

[Juanma Barranquero]

On Mon, Jun 10, 2019 at 12:21 PM Basil L. Contovounesios
<contovob@tcd.ie> wrote:

> FWIW, I find this useful.

Thanks.

> My only questions are a) whether we'd want to
> introduce optional docstring filling more systematically, e.g. during
> *Help* buffer generation,

I don't think so. Usually, docstrings are hand-crafted to be readable,
and random-filling them isn't going to be an improvement per se. In
this specific case, the docstring is mostly auto-generated, with
variable substitution, so filling *is* an improvement. But not
generally.

> and b) whether the fill-column can be
> controlled by emacs-lisp-docstring-fill-column or some new user option
> along the same lines.

IMO, a new option would be unnecessary clutter, but using
emacs-lisp-docstring-fill-column is a good idea. Thanks.


diff --git i/lisp/emacs-lisp/easy-mmode.el w/lisp/emacs-lisp/easy-mmode.el
index be531aab84..2cfbe726e5 100644
--- i/lisp/emacs-lisp/easy-mmode.el
+++ w/lisp/emacs-lisp/easy-mmode.el
@@ -96,9 +96,16 @@ easy-mmode--mode-docstring
     (if (string-match-p "\\bARG\\b" doc)
         doc
-      (let ((argdoc (format easy-mmode--arg-docstring
+      (let ((fill-prefix nil)
+            (fill-column (if (integerp emacs-lisp-docstring-fill-column)
+                             emacs-lisp-docstring-fill-column
+                           65)) ; default value of e-l-d-f-c
+            (argdoc (format easy-mmode--arg-docstring
                             mode-pretty-name)))
-        (replace-regexp-in-string "\\(\n\n\\|\\'\\)\\(.\\|\n\\)*\\'"
-                                  (concat argdoc "\\1")
-                                  doc nil nil 1)))))
+        (with-temp-buffer
+          (insert (replace-regexp-in-string "\\(\n\n\\|\\'\\)\\(.\\|\n\\)*\\'"
+                                            (concat argdoc "\\1")
+                                            doc nil nil 1))
+          (fill-region (point-min) (point-max) 'left t)
+          (buffer-string))))))

 ;;;###autoload

Re: Filling the docstring generated by define-minor-mode

[Stefan Monnier]

> Minor mode's names are often long and the result is quite ugly. An
> easy fix is simply filling the docstring afterwards:

FWIW, I've usually fixed those problems by rewriting the string
so as to make sure the %s is on a "short line".  E.g.

    (defconst easy-mmode--arg-docstring
      "
    
    If called interactively, toggle %s.
    If a prefix ARG is provided, enable the mode if ARG is positive,
    and disable it if ARG is zero or negative.  If called from Lisp,
    also enable the mode if ARG is omitted or nil, and toggle it
    if ARG is `toggle'; disable the mode otherwise.")


-- Stefan

Re: Filling the docstring generated by define-minor-mode

[Juanma Barranquero]

On Mon, Jun 10, 2019 at 7:38 PM Stefan Monnier <monnier@iro.umontreal.ca> wrote:

> FWIW, I've usually fixed those problems by rewriting the string
> so as to make sure the %s is on a "short line".

Fair enough, but the whole docstring, not just the part in
easymode--arg-docstring, undergoes variable substitution.

For example, in this fragment

"Non-nil if %s is enabled.
See the `%s' command
for a description of this minor mode."

it's difficult to reformat it so it looks good with either very short
or very long mode names.

RE: Filling the docstring generated by define-minor-mode

[Drew Adams]

(Caveat: Haven't been following this thread.)

> > FWIW, I've usually fixed those problems by rewriting the string
> > so as to make sure the %s is on a "short line".
> 
> Fair enough, but the whole docstring, not just the part in
> easymode--arg-docstring, undergoes variable substitution.
> 
> For example, in this fragment
> 
> "Non-nil if %s is enabled.
> See the `%s' command
> for a description of this minor mode."
> 
> it's difficult to reformat it so it looks good with either very short
> or very long mode names.

 Non-nil if you have enabled
 %s.

 For a description of this minor mode see command
 `%s'.

Maybe more than such a change is needed.  I don't
know what gets substituted for %s here.

Anyway, put me in the camp that thinks that the
solution is manually rewriting the doc string.
Blindly filling such stuff automatically asks for
trouble, IMO.

Re: Filling the docstring generated by define-minor-mode

[Stefan Monnier]

> Fair enough, but the whole docstring, not just the part in
> easymode--arg-docstring, undergoes variable substitution.

Yes, doing it manually sometimes result in awkward results.
Also, it's easy to forget one aspect.

Filling is probably a better solution in that it's more general (of
course, that presumes there's no embedded code examples which could be
mis-filled, but in this particular case it seems like a non-issue).


        Stefan

Re: Filling the docstring generated by define-minor-mode

[Juanma Barranquero]

On Mon, Jun 10, 2019 at 8:33 PM Stefan Monnier <monnier@iro.umontreal.ca> wrote:

> Filling is probably a better solution in that it's more general (of
> course, that presumes there's no embedded code examples which could be
> mis-filled, but in this particular case it seems like a non-issue).

That's why I think is a good fit for this particular case, but not for
filling docstrings as a rule.

Re: Filling the docstring generated by define-minor-mode

[Juanma Barranquero]

On Mon, Jun 10, 2019 at 8:24 PM Drew Adams <drew.adams@oracle.com> wrote:

> Maybe more than such a change is needed.  I don't
> know what gets substituted for %s here.

The minor mode name, mostly. Which can be a single, short word, or as
long as display-fill-column-indicator, or more. Which makes finding a
good answer for any such format impractical.

> Blindly filling such stuff automatically asks for
> trouble, IMO.

Generally speaking, I agree.

RE: Filling the docstring generated by define-minor-mode

[Drew Adams]

> > Maybe more than such a change is needed.  I don't
> > know what gets substituted for %s here.
> 
> The minor mode name, mostly. Which can be a single, short word, or as
> long as display-fill-column-indicator, or more. Which makes finding a
> good answer for any such format impractical.

Presumably you mean `display-fill-column-indicator-mode',
which is even worse.

> > Blindly filling such stuff automatically asks for
> > trouble, IMO.
> 
> Generally speaking, I agree.

If we impose that here then a user defining a minor
mode has zero control over it.  If we don't fill
blindly then users have control.  They can do what
they think is best.

For such generated text, I think, it's best to use
text that doesn't require mentioning the particular
name at all, or mentions it just once, on a separate
line.

Trying to inject unknown names (let alone unknown
arbitrary text) into some boilerplate text, and then
filling the result, is a recipe for bad results.
And it's not really needed, is it?

The user's part of the doc string should be under
her control, and the generated part should preferably
not inject (i.e., %s) any text at all.  That should
be doable, I think.

Re: Filling the docstring generated by define-minor-mode

[Juanma Barranquero]

> Presumably you mean `display-fill-column-indicator-mode',
> which is even worse.

The name of the mode is display-fill-column-indicator, or Display Fill
Column Indicator. With -mode, it is the name of the mode's function,
or variable. But this is a moot point.

> If we impose that here then a user defining a minor
> mode has zero control over it.  If we don't fill
> blindly then users have control.  They can do what
> they think is best.

The mode creater has control. If you pass a doc to
'define-minor-mode', it doesn't generate one for you. But if you pass
nil, then it generates one, and you don't have control, with or
without my change. What I propose does not affect the mode creator's
control, only the formatting of an autogenerated docstring they can
opt out of ;-)

> For such generated text, I think, it's best to use
> text that doesn't require mentioning the particular
> name at all, or mentions it just once, on a separate
> line.

I'm not trying to find a better docstring. I tend to assume the
docstring used for many minor modes will have been already discussed
and agreed upon long time ago.

> The user's part of the doc string should be under
> her control, and the generated part should preferably
> not inject (i.e., %s) any text at all.  That should
> be doable, I think.

You can have all the control you want, if you write the docstring
instead of relying on autogeneration.

Re: Filling the docstring generated by define-minor-mode

[Stefan Monnier]

>> Filling is probably a better solution in that it's more general (of
>> course, that presumes there's no embedded code examples which could be
>> mis-filled, but in this particular case it seems like a non-issue).
> That's why I think is a good fit for this particular case,

Agreed.

> but not for filling docstrings as a rule.

[ Actually, I think we should aim for automatic refilling in the
  longer run.  That means using a format that unambiguously distinguishes
  prose from preformatted contents.  But that's another discussion.  ]


        Stefan

Re: Filling the docstring generated by define-minor-mode

[Juanma Barranquero]

On Tue, Jun 11, 2019 at 9:14 AM Stefan Monnier <monnier@iro.umontreal.ca> wrote:

> [ Actually, I think we should aim for automatic refilling in the
>   longer run.  That means using a format that unambiguously distinguishes
>   prose from preformatted contents.  But that's another discussion.  ]

FWIW, I agree. Back when the argument highlighting for function
docstrings was implemented, there was a bit of talk about adding some
kind of markup to docstrings, but no consensus emerged.

I'm all for adding markup to docstrings so we can use a variable-pitch
font for normal descriptions and fixed-pitch for code, have the
ability to add tables and images to docstrings,  use bold and italic
and faces etc if/when it would help (I'm not advocating for angry
fruit salad, at least by default). With these facilities, auto-filling
of normal paragraphs would make sense.

But certainly, that's another discussion.

BTW, do we have a way to do "visual" filling, I mean, fill
variable-pitch text up to a given (visual) width?

     Juanma

Re: Filling the docstring generated by define-minor-mode

[Stefan Monnier]

> BTW, do we have a way to do "visual" filling, I mean, fill
> variable-pitch text up to a given (visual) width?

Not sure whether "have a way" applies, but shr.el does have code which
does just that.  I haven't looked to see how heroic was Lars's effort to
get that working.


        Stefan

Re: Filling the docstring generated by define-minor-mode

[Juanma Barranquero]

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

Revisiting this issue...

I proposed to automatically refill the docstrings that are auto-generated
by define-minor-mode. Hand-crafted docstrings, or other doc strings outside
define-minor-mode's ones are not affected.

Obviously, as the one who proposed it, I think this is a good change for
that particular problem (and that particular problem *only*), regardless of
whether we will want to add more formatting or refilling capabilities for
docstrings in the future.

So I'd like to get a definite "ok" or "not ok" to close this issue.


diff --git i/lisp/emacs-lisp/easy-mmode.el w/lisp/emacs-lisp/easy-mmode.el
index be531aab84..fe89ed1754 100644
--- i/lisp/emacs-lisp/easy-mmode.el
+++ w/lisp/emacs-lisp/easy-mmode.el
@@ -96,9 +96,15 @@ easy-mmode--mode-docstring
     (if (string-match-p "\\bARG\\b" doc)
         doc
-      (let ((argdoc (format easy-mmode--arg-docstring
+      (let ((fill-prefix nil)
+            (docstring-fc (bound-and-true-p
emacs-lisp-docstring-fill-column))
+            (fill-column (if (integerp docstring-fc) docstring-fc 65))
+            (argdoc (format easy-mmode--arg-docstring
                             mode-pretty-name)))
-        (replace-regexp-in-string "\\(\n\n\\|\\'\\)\\(.\\|\n\\)*\\'"
-                                  (concat argdoc "\\1")
-                                  doc nil nil 1)))))
+        (with-temp-buffer
+          (insert (replace-regexp-in-string
"\\(\n\n\\|\\'\\)\\(.\\|\n\\)*\\'"
+                                            (concat argdoc "\\1")
+                                            doc nil nil 1))
+          (fill-region (point-min) (point-max) 'left t)
+          (buffer-string))))))

 ;;;###autoload

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

Re: Filling the docstring generated by define-minor-mode

[Lars Ingebrigtsen]

Juanma Barranquero <lekktu@gmail.com> writes:

> I proposed to automatically refill the docstrings that are auto-generated by
> define-minor-mode. Hand-crafted docstrings, or other doc strings outside
> define-minor-mode's ones are not affected.

I think this is a good change, because those auto-generated sections
look pretty messy now.

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

Re: Filling the docstring generated by define-minor-mode

[Stefan Monnier]

> So I'd like to get a definite "ok" or "not ok" to close this issue.

OK on my side,


        Stefan

Re: Filling the docstring generated by define-minor-mode

[Juanma Barranquero]

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

Committed as a397fa06d18d6ae37a3a1288f269e1ae9eb3b569

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