Subject: Re: bug#51809: 29.0.50; [PATCH] Support for outline default state in Diff buffers

From: Eli Zaretskii <[email-masked]>
Date: Sun, 26 Dec 2021 18:21:42 +0200

> From: Matthias Meulien <[email-masked]>
> Date: Sun, 26 Dec 2021 17:05:25 +0100
> Cc: [email-masked]
> 
> Here is an updated patch implementing a default state for Outline mode
> and Outline minor mode:

Thanks, a few comments to the documentation parts:

> +If equal to a number, show only headings up to the corresponding
> +level.

"Up to and including" or "up to and excluding"?

Also, please make sure you leave 2 spaces between sentences in all the
doc strings and comments, per our conventions.

> +  :type '(choice (const :tag "Disabled" nil)

It isn't clear what exactly is "disabled" under this value.  The doc
string itself is also not clear about what happens when the value is
nil: "some headings are outlined" leaves me wondering what that means.
> +(defcustom outline-default-state-subtree-visibility nil
> +  "Defines visibility of subtree starting at level defined by `outline-default-state'.

Not "Defines", "Determines".

Also, the first line is too long.

> +When equal to a list, each element is expected to equal one of:
                                      ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
"...should be one of the following:"

(Using "equal" here is not appropriate in any case, since you don't
always describe actual values.)

> +- A lambda function or function name which will be evaluated with
> +  point at the beginningg of the heading and the match data set
                  ^^^^^^^^^^
A typo.

And finally, I think this should have a NEWS entry.