GNU bug report logs - #37410
[PATCH] Several doc fixes in package.el

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 37410 in the body.
You can then email your comments to 37410 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Stefan Kangas <stefan <at> marxist.se>
To: bug-gnu-emacs <at> gnu.org
Subject: [PATCH] Several doc fixes in package.el
Date: Sun, 15 Sep 2019 18:01:17 +0200

[Message part 1 (text/plain, inline)]

Seeing as the documentation in package.el leaves much to be desired, I
spent some time adding doc strings and fixing checkdoc and stylistic
errors.  I've attached a patch with my results, which should improve
the situation a little bit at least.  Is this okay to install?

Best regards,
Stefan Kangas

[0001-Several-doc-fixes-in-package.el.patch (text/x-patch, attachment)]

Message #8 received at 37410 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Stefan Kangas <stefan <at> marxist.se>
Cc: 37410 <at> debbugs.gnu.org
Subject: Re: bug#37410: [PATCH] Several doc fixes in package.el
Date: Sun, 15 Sep 2019 19:37:14 +0300

> From: Stefan Kangas <stefan <at> marxist.se>
> Date: Sun, 15 Sep 2019 18:01:17 +0200
> 
> Seeing as the documentation in package.el leaves much to be desired, I
> spent some time adding doc strings and fixing checkdoc and stylistic
> errors.  I've attached a patch with my results, which should improve
> the situation a little bit at least.

Thanks for working on this.

>  (defun package-check-signature ()
>    "Check whether we have a usable OpenPGP configuration.
> -If true, and `package-check-signature' is `allow-unsigned',
> -return `allow-unsigned', otherwise return the value of
> -`package-check-signature'."
> +If true, and variable `package-check-signature' is

"True" is inappropriate here.  I'd use "If so, and..." instead.

>  (defun package--from-builtin (bi-desc)
> +  "Create a `package-desc' object from BI-DESC.
> +BI-DESC should be an `package--bi-desc' object."
                     ^^
"a"

>  (defun package-desc-full-name (pkg-desc)
> +  "Return full name of package-desc object PKG-DESC.
> +For example, if the package is named \"foo\" and has version
> +\"1.2.3\", then return \"foo-1.2.3\"."

Instead of "for example", it is better to tell explicitly whet this
does.  E.g.,:

    "Return full name of package-desc object PKG-DESC.
  This is the name of the package with its version appended."

>  (defun package-desc-suffix (pkg-desc)
> +  "Return suffix of package-desc object PKG-DESC.

I'd say "file-name extension" instead of "suffix".

>  (defun package-desc--keywords (pkg-desc)
> +  "Return keywords of package-desc object PKG-DESC."

Suggest to say something about what these keywords are and where they
come from.  Otherwise, "keywords" is such a vague term that it's
impossible to understand that without reading the code.

> +Convert EXP into a `package-desc' object using the
> +`package-desc-from-define' constructor before pushing it to
> +`package-alist.
                  ^
A closing quote missing there.

>  (defun package-archive-base (desc)
> -  "Return the archive containing the package NAME."
> +  "Return the archive containing the package DESC."

I'd say "the package described by DESC".

>  (defun package-install-from-buffer ()
> -  "Install a package from the current buffer.
> +  "Install package from current buffer.

Why this change?

>  ;;;###autoload
>  (defun package-install-file (file)
> -  "Install a package from a file.
> +  "Install package from FILE.

And this?

>  (defun describe-package-1 (pkg)
> +  "Insert package description of PKG at point.
> +Helper function for `describe-package'."

The "at point" here is ambiguous: does it mean "insert at point" or
"PKG at point"?

>  (defun package-install-button-action (button)
> +  "Run `package-install' on package defined by BUTTON.

Can a package really be defined by a button?

>  (defun package-keyword-button-action (button)
> +  "Show *Packages* buffer filtered by keyword from BUTTON label.

*Packages* should be in double quotes.

I generally find this sentence confusing: what do you mean by "keyword
from BUTTON label"?

> +(defun package-make-button (text &rest properties)
> +  "Insert button labelled TEXT with button PROPERTIES at point.
                    ^^^^^^^^
"labeled"

>  (defun package--print-email-button (name)
> +  "Insert a button to email NAME at point.

"To email NAME" is confusing.  I'd suggest to rename it ADDRESSEE.
"Insert a button to email" is also confusing.  Is this alternative
correct?

  Insert a button whose action will send email to ADDRESSEE.

> +NAME should have the form (FULLNAME . EMAIL) where NAME is either
                                                      ^^^^
FULLNAME

>  (defvar package-list-unversioned nil
> -  "If non-nil include packages that don't have a version in `list-package'.")
> +  "If non-nil, include packages that don't have a version in `list-package'.")
                                                                 ^^^^^^^^^^^^
"list-packages", I presume?

>  (defvar package--emacs-version-list (version-to-list emacs-version)
> -  "`emacs-version', as a list.")
> +  "Variable `emacs-version' as a list.")

"The value of `emacs-version', as a list."

Message #11 received at 37410 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Kangas <stefan <at> marxist.se>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 37410 <at> debbugs.gnu.org
Subject: Re: bug#37410: [PATCH] Several doc fixes in package.el
Date: Sun, 15 Sep 2019 21:56:48 +0200

[Message part 1 (text/plain, inline)]

Eli Zaretskii <eliz <at> gnu.org> writes:

>> From: Stefan Kangas <stefan <at> marxist.se>
>> Date: Sun, 15 Sep 2019 18:01:17 +0200
>>
>> Seeing as the documentation in package.el leaves much to be desired, I
>> spent some time adding doc strings and fixing checkdoc and stylistic
>> errors.  I've attached a patch with my results, which should improve
>> the situation a little bit at least.
>
> Thanks for working on this.

Thanks for your detailed review.  I have attached an updated patch.

>>  (defun package-install-from-buffer ()
>> -  "Install a package from the current buffer.
>> +  "Install package from current buffer.
>
> Why this change?

Reverted that.

>>  ;;;###autoload
>>  (defun package-install-file (file)
>> -  "Install a package from a file.
>> +  "Install package from FILE.
>
> And this?

I think the original is wordy for no reason, and imprecise: We do not
install just "a package" in general, but specifically the package
pointed to by FILE.  But if I'm the only one who feels that the terse
version is better, I'm willing to concede that point.

>>  (defun describe-package-1 (pkg)
>> +  "Insert package description of PKG at point.
>> +Helper function for `describe-package'."
>
> The "at point" here is ambiguous: does it mean "insert at point" or
> "PKG at point"?

Changed that to:

    Insert the package description for PKG.

>>  (defun package-install-button-action (button)
>> +  "Run `package-install' on package defined by BUTTON.
>
> Can a package really be defined by a button?

Changed that to:

    Run `package-install' on the package BUTTON points to.

>>  (defun package-keyword-button-action (button)
>> +  "Show *Packages* buffer filtered by keyword from BUTTON label.
>
> *Packages* should be in double quotes.
>
> I generally find this sentence confusing: what do you mean by "keyword
> from BUTTON label"?

Changed that to:

+  "Show filtered \"*Packages*\" buffer for BUTTON.
+The buffer is filtered by the `package-keyword' property of BUTTON.

>> +(defun package-make-button (text &rest properties)
>> +  "Insert button labelled TEXT with button PROPERTIES at point.
>                     ^^^^^^^^
> "labeled"

Right, I used the British spelling by mistake.  Fixed.

>>  (defun package--print-email-button (name)
>> +  "Insert a button to email NAME at point.
>
> "To email NAME" is confusing.  I'd suggest to rename it ADDRESSEE.
> "Insert a button to email" is also confusing.  Is this alternative
> correct?

"Insert a button" is from the doc string of insert-text-button (for
which this is a wrapper) which says:

    "Insert a button with the label LABEL.#

>   Insert a button whose action will send email to ADDRESSEE.

Better, but I changed it to RECIPIENT instead of ADDRESSEE.

>>  (defvar package--emacs-version-list (version-to-list emacs-version)
>> -  "`emacs-version', as a list.")
>> +  "Variable `emacs-version' as a list.")
>
> "The value of `emacs-version', as a list."

Fixed.  (FWIW, I don't know what the point of this defvar is -- it's
only used once as far as I can tell.  Maybe it should just be removed.)

Best regards,
Stefan Kangas

[0001-Several-doc-fixes-in-package.el.patch (text/x-patch, attachment)]

Message #16 received at 37410-done <at> debbugs.gnu.org (full text, mbox):

From: Stefan Kangas <stefan <at> marxist.se>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 37410-done <at> debbugs.gnu.org
Subject: Re: bug#37410: [PATCH] Several doc fixes in package.el
Date: Sat, 21 Sep 2019 23:14:35 +0200

Stefan Kangas <stefan <at> marxist.se> writes:

> Thanks for your detailed review.  I have attached an updated patch.

No more comments in a week, so I've now pushed this as commit b86bc62ca5.

> >>  (defun package-install-file (file)
> >> -  "Install a package from a file.
> >> +  "Install package from FILE.
> >
> > And this?
>
> I think the original is wordy for no reason, and imprecise: We do not
> install just "a package" in general, but specifically the package
> pointed to by FILE.  But if I'm the only one who feels that the terse
> version is better, I'm willing to concede that point.

In the end, I stuck with:

    "Install a package from FILE."

Best regards,
Stefan Kangas

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.