Subject: Re: bug#21074: [PATCH] Add docs for two tabulated-list functions

From: Eli Zaretskii <[email-masked]>
Date: Sat, 02 Feb 2019 18:33:01 +0200

> From: Alex Branham <[email-masked]>
> Cc: [email-masked], [email-masked]
> Date: Sat, 02 Feb 2019 10:03:31 -0600
> 
> Thanks. I think the attached patch incorporates all your comments, let
> me know if I missed something.

Looks good.  A couple of minor gotchas, though.

> * doc/lispref/modes.texi: Add documentation for
>   'tabulated-list-delete-entry', 'tabulated-list-get-id',
>   'tabulated-list-get-entry', 'tabulated-list-header-overlay-p',
>   'tabulated-list-put-tag', and 'tabulated-list-set-col'.

This should mention the bug number.

> +@defun tabulated-list-delete-entry
> +This function deletes the entry at point.
> +
> +It returns a list @code{(id cols)} where @var{id} is the ID of the
                                     ^
A comma is missing there.

> +delete entry and @var{cols} is a vector of its column descriptors.
               ^
And here.

Also, the "id" and "cols" inside the list should also have the @var
markup.

> +It moves point to the beginning of the deleted entry.

The last sentence is confusing: if the entry is deleted, how can we
move to its beginning?

> +@defun tabulated-list-header-overlay-p &optional POS
> +This @code{defsubst} returns non-nil if there is a fake header at
> +@var{pos}.

We should explain, in a single sentence if possible, what is a "fake
header".  It is never explained in this section.

> +@defun tabulated-list-put-tag tag &optional advance
> +This function puts @var{tag} in the padding area of the current line.

And this should explain what is the padding area, for the same reason.

> +@var{tag} should be a string, with a length less than or equal to
> +@code{tabulated-list-padding}.

Every variable mentioned in the manual should be indexed.  So please
add

 @vindex tabulated-list-padding

before the @defun.

> +If @var{change-entry-data} is non-nil, this function modifies the
> +underlying entry data by setting the appropriate slot of the vector
> +originally used to print this entry.  If @code{tabulated-list-entries}
> +has a list value, this is the vector stored within it.

This paragraph is confusing, I cannot understand what that argument
does just by reading the above text.  (The doc string says the same,
so it's of no help.)  The confusing parts are "appropriate slot" and
"originally used to print".  The code simply modifies a component of
the vector returned by tabulated-list-get-entry, so I wonder why the
description needs to be that complicated.

Thanks again for working on this.