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

Previous Next

Full log

View this message in rfc822 format

From: Eli Zaretskii <eliz <at> gnu.org>
To: Stefan Kangas <stefan <at> marxist.se>
Cc: 37410 <at> debbugs.gnu.org
Subject: 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."

Previous Next

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