Subject: bug#72357: Checkdoc fixes in transient.el

From: Eli Zaretskii <[email-masked]>
Date: Tue, 30 Jul 2024 14:39:53 +0300

> 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" <[email-masked]>
> 
> 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."