On Thu, 26 Jan 2012 20:17:49 +0400, Dmitry Kurochkin <dmitry.kurochkin@gmail.com> wrote:
> I did not review the code, but here is a general comment for both
> patches (but especially for the first one).  It would be nice to have a
> more detailed documentation for hooks.  Docstring like "Enable Visual
> Line mode." for a function named `notmuch-show-turn-on-visual-line-mode'
> is near useless.  It is quite obvious that the function enables
> visual-line-mode from it's name.  And it does not give any information
> on why would someone actually want to use it.  I do not remember what
> visual-line-mode is exactly, so to understand whether this hook is
> actually useful for me, I have to read visual-line-mode docs, think
> about how it helps in notmuch-show, read some code, perhaps, etc.  I
> would argue that since the hook itself is trivial, the main point in
> having it is to provide a clearly documented solution for a common
> problem for those who do not know how to solve this problem right away.
> Currently, those who know what visual-line-mode is do not need this
> hook, because they can easily write their own, and those who do not know
> what visual-line-mode is can not use this hook, because it says nothing
> about why it is actually useful.
> 
> Also, in addition to better docs, I would rename
> `notmuch-show-turn-on-visual-line-mode' to something that reflects what
> it does from user POV (like the other two hooks).
> 
> Though, the fact that the hook is enabled by default makes the above
> arguments less important, I guess.

I have a bunch of somewhat conflicting thoughts about this:
  - Being able to configure the behaviour without having to change the
    core code is good, so implementing behaviour using hook functions is
    useful.
  - Things should behave well in the default configuration, so most of
    the hook functions are enabled by default.
  - Everything can't be hook functions, so there's a balance to be made
    between implementing things as in-line code and via hook functions.
  - Most users shouldn't need to modify any of the hooks.
  - Documentation that explains what a hook function is about is
    obviously good.
  - Documenting something that is external to notmuch can be both
    wasteful and risky.

    Wasteful because such documentation typically already exists and
    risky because the precise behaviour of external components is not
    under our control.

    For example, the documentation for `visual-line-mode' is both
    concise and good, so there's little point in repeating it and, of
    course, the exact details of what `visual-line-mode' does can be
    changed by the Emacs developers. One would expect that they would
    update the documentation for `visual-line-mode' in such situations,
    which would leave any cloned documentation in an incorrect state.

Hence, I would probably take issue with your statement:

> I would argue that since the hook itself is trivial, the main point in
> having it is to provide a clearly documented solution for a common
> problem for those who do not know how to solve this problem right
> away.

That's not the intention of the hook functions under discussion
here. They are hook functions so that a curious and interested user can
make a change based on some aesthetic preference. The are not about
solving problems with the default configuration.

I think that `turn-on-visual-line-mode' (or the package local derivative
of it that was chosen) is precisely the right name for a function that
enables `visual-line-mode'. It describes perfectly and succinctly what
the function does and provides a pointer to follow to find out more (the
documentation for `visual-line-mode') without a user even having to
examine the documentation of the function itself.

All of that said, I'd agree that the documentation of
`notmuch-show-indent-continuations' could have been better. How about:
  "Indent any continuation lines in the current headers."
?