bug#72357: Checkdoc fixes in transient.el

bug#72357: Checkdoc fixes in transient.el

[Jonas Bernoulli via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Hello Stefan,

I don't think your commit e3bba63ecb9 is an improvement.

I generally agree that the first line of a docstring should be a short
sentence, and that no other sentence should start on that line, even if
there is plenty of space left.  In fact I have contributed many similar
fixes to package maintained by other people.

However, I would argue that the usual reasoning for why one should do
that, does not apply here.  In this case, the first line won't appear
anywhere by itself, without the rest of this docstring (such as in
apropos output), because this is a method not a "regular" or generic
function.

Looking at, for example, "C-h f transient-format-description", I feel
that it would not make sense if all the methods themselves began with a
summary line.  Only the overall generic function *needs* a summary line.
In some case it may make sense to give individual methods their own
summary lines, but for very short, one paragraph method docstrings this
should not be a requirements.  When a method is so simple that it can be
described using a single short paragraph (but not a single sentence,
which can fit on a single line), then that should be possible, without
being forced to mess up the justification of that paragraph.

IMO checkdoc should be updated to not enforce the conventions, which
were designed for "top-level" functions (and variables) on methods
as well.

Also consider the case where a method can be described using a single
sentence, but that sentence requires two lines.  Forcing the author
to prefix that short paragraph with a sorter sentence, which only
serves to satisfies an ill-applied convention, feels wrong to me.

---

In this particular case, separating the first sentence from the rest of
the paragraph (but without completely rewording the paragraph) is a step
backward.

Each of these method docstrings consist of two sentences.  The first
sentence is very much not a summary of the second sentence.  Each of
these methods does two things and each thing is described using one
sentence.  The important thing, the one that makes the method useful, is
described in the second sentence.  The boring thing (call the next
method) is described in the first sentence, because it is done first.
Using the first sentence as the "summary" is wrong in these cases.
Changing the order in which the two steps are described, would likely
lead to awkward wording.

     Jonas

bug#72357: Checkdoc fixes in transient.el

[Eli Zaretskii]

> Date: Mon, 29 Jul 2024 23:56:00 +0200
> From:  Jonas Bernoulli via "Bug reports for GNU Emacs,
>  the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org>
> 
> Hello Stefan,
> 
> I don't think your commit e3bba63ecb9 is an improvement.

Thanks for your feedback.

However, I tend to disagree with you, see below.

> I generally agree that the first line of a docstring should be a short
> sentence, and that no other sentence should start on that line, even if
> there is plenty of space left.  In fact I have contributed many similar
> fixes to package maintained by other people.
> 
> However, I would argue that the usual reasoning for why one should do
> that, does not apply here.  In this case, the first line won't appear
> anywhere by itself, without the rest of this docstring (such as in
> apropos output), because this is a method not a "regular" or generic
> function.
> 
> Looking at, for example, "C-h f transient-format-description", I feel
> that it would not make sense if all the methods themselves began with a
> summary line.  Only the overall generic function *needs* a summary line.
> In some case it may make sense to give individual methods their own
> summary lines, but for very short, one paragraph method docstrings this
> should not be a requirements.  When a method is so simple that it can be
> described using a single short paragraph (but not a single sentence,
> which can fit on a single line), then that should be possible, without
> being forced to mess up the justification of that paragraph.
> 
> IMO checkdoc should be updated to not enforce the conventions, which
> were designed for "top-level" functions (and variables) on methods
> as well.
> 
> Also consider the case where a method can be described using a single
> sentence, but that sentence requires two lines.  Forcing the author
> to prefix that short paragraph with a sorter sentence, which only
> serves to satisfies an ill-applied convention, feels wrong to me.

These are all general considerations.  They do make sense, but IME
they should be discussed with specific cases in mind, because what
exactly is the best path in each case is a judgment call, which is
sometimes resolved due to very fine nuances.

If we now turn to the practical aspects of this, then first I'd like
to point out that the commit in question touched only 2 doc strings,
which were both very similar in their wording, and which IMO could use
some improvements.  Just to put this on the table, here's one of the
two changes (the other one is almost identical):

   (cl-defmethod transient-format-description ((obj transient-group))
  -  "Format the description by calling the next method.  If the result
  -doesn't use the `face' property at all, then apply the face
  -`transient-heading' to the complete string."
  +  "Format the description by calling the next method.
  +If the result doesn't use the `face' property at all, then apply the
  +face `transient-heading' to the complete string."

> In this particular case, separating the first sentence from the rest of
> the paragraph (but without completely rewording the paragraph) is a step
> backward.

IME, if done right, it is never a step backward.

> Each of these method docstrings consist of two sentences.  The first
> sentence is very much not a summary of the second sentence.  Each of
> these methods does two things and each thing is described using one
> sentence.  The important thing, the one that makes the method useful, is
> described in the second sentence.  The boring thing (call the next
> method) is described in the first sentence, because it is done first.
> Using the first sentence as the "summary" is wrong in these cases.
> Changing the order in which the two steps are described, would likely
> lead to awkward wording.

I agree that the current doc strings can be improved.  But I don't
agree that the original doc strings were better.  Their first sentence
basically says very little about what it does and leaves a lot unsaid
(what is "the next method"? what is "the description"?), and the next
sentence doesn't clarify that (instead, it adds some detail about what
the method does).  The doc string of the corresponding cl-defgeneric
doesn't help, either.

So IMO these doc strings "need some work"(TM).  In particular, if you
say that the important part of the description is in the second
sentence, then that sentence should be the first one.  (The sentence
that is currently the first I'm not sure should be there at all.)

Also, I think the doc string should mention the types of arguments to
which the method is applicable (right now, it says absolutely nothing
about that).  Preferably, this should be in the first line/sentence of
the doc string.

If you can describe in a few more words what one of these methods do,
I can try suggesting how to reword its doc string, and that will
hopefully illustrate what I mean above in practical terms.  For now,
here's the first attempt based on just looking at the code:

  (cl-defmethod transient-format-description ((obj transient-group))
    "Format the description of `transient-group' object OBJ using faces.
  If the description of OBJ already uses faces, return it unchanged.
  Otherwise, apply the `transient-inapt-suffix' face to the entire
  description stribng if the OBJ's inapt slot is non-nil, else
  the `transient-heading' face."

bug#72357: Checkdoc fixes in transient.el

[Stefan Kangas]

Jonas Bernoulli <jonas@bernoul.li> writes:

> Looking at, for example, "C-h f transient-format-description", I feel
> that it would not make sense if all the methods themselves began with a
> summary line.  Only the overall generic function *needs* a summary line.
> In some case it may make sense to give individual methods their own
> summary lines, but for very short, one paragraph method docstrings this
> should not be a requirements.  When a method is so simple that it can be
> described using a single short paragraph (but not a single sentence,
> which can fit on a single line), then that should be possible, without
> being forced to mess up the justification of that paragraph.

Sure, I understand your reasoning.  I don't know if this needs pointing
out, but do feel free to revert these changes.

Meanwhile, Eli has proposed a rewording of the docstring that you might
want to look at.

> IMO checkdoc should be updated to not enforce the conventions, which
> were designed for "top-level" functions (and variables) on methods
> as well.

Makes sense to me.

BTW, with debbugs, it's better to put people in the "X-Debbugs-CC"
header than "Cc".  That way, I get the email forwarded to me by the bug
tracker, and when I hit "wide reply", I send it to the right bug
automatically (I had to edit in manually here).