bug#37410: [PATCH] Several doc fixes in package.el

bug#37410: [PATCH] Several doc fixes in package.el

[Stefan Kangas]

[-- Attachment #1: Type: text/plain, Size: 301 bytes --]

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

[-- Attachment #2: 0001-Several-doc-fixes-in-package.el.patch --]
[-- Type: text/x-patch, Size: 12867 bytes --]

From a5c324028dd001a3a759534b99866afa05a2535b Mon Sep 17 00:00:00 2001
From: Stefan Kangas <stefankangas@gmail.com>
Date: Sun, 15 Sep 2019 17:41:27 +0200
Subject: [PATCH] Several doc fixes in package.el

* lisp/emacs-lisp/package.el (top-level)
(package-check-signature, package--from-builtin)
(package-desc-full-name, package-desc-suffix)
(package-desc--keywords, package--bi-desc)
(package-process-define-package, package-archive-base)
(package-install-from-archive, package-install-from-buffer)
(package-install-file, package-autoremove, describe-package-1)
(package-install-button-action, package-delete-button-action)
(package-keyword-button-action, package-make-button)
(package--print-email-button, package-list-unversioned)
(package--emacs-version-list, package-menu-toggle-hiding)
(package-hidden-regexps, package-menu-hide-package)
(package-menu-get-status, package-menu--find-upgrades)
(package-menu--post-refresh): Doc fixes.
---
 lisp/emacs-lisp/package.el | 85 +++++++++++++++++++++++++++++---------
 1 file changed, 65 insertions(+), 20 deletions(-)

diff --git a/lisp/emacs-lisp/package.el b/lisp/emacs-lisp/package.el
index ef0c5171de..edc62c5394 100644
--- a/lisp/emacs-lisp/package.el
+++ b/lisp/emacs-lisp/package.el
@@ -120,9 +120,9 @@
 ;; - "installed" instead of a blank in the status column
 ;; - tramp needs its files to be compiled in a certain order.
 ;;   how to handle this?  fix tramp?
-;; - maybe we need separate .elc directories for various emacs versions
-;;   and also emacs-vs-xemacs.  That way conditional compilation can
-;;   work.  But would this break anything?
+;; - maybe we need separate .elc directories for various emacs
+;;   versions.  That way conditional compilation can work.  But would
+;;   this break anything?
 ;; - William Xu suggests being able to open a package file without
 ;;   installing it
 ;; - Interface with desktop.el so that restarting after an install
@@ -354,9 +354,9 @@ package-check-signature
 
 (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
+`allow-unsigned', return `allow-unsigned', otherwise return the
+value of variable `package-check-signature'."
   (if (eq package-check-signature 'allow-unsigned)
       (progn
         (require 'epg-config)
@@ -472,6 +472,8 @@ package--default-summary
   signed)
 
 (defun package--from-builtin (bi-desc)
+  "Create a `package-desc' object from BI-DESC.
+BI-DESC should be an `package--bi-desc' object."
   (package-desc-create :name (pop bi-desc)
                        :version (package--bi-desc-version bi-desc)
                        :summary (package--bi-desc-summary bi-desc)
@@ -509,11 +511,22 @@ package-version-join
       (apply #'concat (nreverse str-list)))))
 
 (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\"."
   (format "%s-%s"
           (package-desc-name pkg-desc)
           (package-version-join (package-desc-version pkg-desc))))
 
 (defun package-desc-suffix (pkg-desc)
+  "Return suffix of package-desc object PKG-DESC.
+Depending on the `package-desc-kind' of PKG-DESC, this is one of:
+
+   'single - \".el\"
+   'tar    - \".tar\"
+   'dir    - \"\"
+
+If the kind is none of the above, signal an error."
   (pcase (package-desc-kind pkg-desc)
     ('single ".el")
     ('tar ".tar")
@@ -521,6 +534,7 @@ package-desc-suffix
     (kind (error "Unknown package kind: %s" kind))))
 
 (defun package-desc--keywords (pkg-desc)
+  "Return keywords of package-desc object PKG-DESC."
   (let ((keywords (cdr (assoc :keywords (package-desc-extras pkg-desc)))))
     (if (eq (car-safe keywords) 'quote)
         (nth 1 keywords)
@@ -530,10 +544,10 @@ package-desc-priority
   "Return the priority of the archive of package-desc object P."
   (package-archive-priority (package-desc-archive p)))
 
-;; Package descriptor format used in finder-inf.el and package--builtins.
 (cl-defstruct (package--bi-desc
                (:constructor package-make-builtin (version summary))
                (:type vector))
+  "Package descriptor format used in finder-inf.el and package--builtins."
   version
   reqs
   summary)
@@ -575,7 +589,15 @@ package-activated-list
 ;; The following functions are called on each installed package by
 ;; `package-load-all-descriptors', which ultimately populates the
 ;; `package-alist' variable.
+
 (defun package-process-define-package (exp)
+  "Process define-package expression EXP and push it to `package-alist'.
+EXP should be a form read from a foo-pkg.el file.
+Convert EXP into a `package-desc' object using the
+`package-desc-from-define' constructor before pushing it to
+`package-alist.
+If there already exists a package by that name in
+`package-alist', replace that definition with the new one."
   (when (eq (car-safe exp) 'define-package)
     (let* ((new-pkg-desc (apply #'package-desc-from-define (cdr exp)))
            (name (package-desc-name new-pkg-desc))
@@ -866,6 +888,7 @@ package--alist-to-plist-args
   (mapcar #'macroexp-quote
           (apply #'nconc
                  (mapcar (lambda (pair) (list (car pair) (cdr pair))) alist))))
+
 (defun package-unpack (pkg-desc)
   "Install the contents of the current buffer as a package."
   (let* ((name (package-desc-name pkg-desc))
@@ -1900,11 +1923,11 @@ package--sort-by-dependence
 ;; requirements (dependencies) are always satisfied by looking in
 ;; `package-archive-contents'.
 (defun package-archive-base (desc)
-  "Return the archive containing the package NAME."
+  "Return the archive containing the package DESC."
   (cdr (assoc (package-desc-archive desc) package-archives)))
 
 (defun package-install-from-archive (pkg-desc)
-  "Download and install a tar package."
+  "Download and install a tar package defined by PKG-DESC."
   ;; This won't happen, unless the archive is doing something wrong.
   (when (eq (package-desc-kind pkg-desc) 'dir)
     (error "Can't install directory package from archive"))
@@ -2043,7 +2066,7 @@ package-strip-rcs-id
 
 ;;;###autoload
 (defun package-install-from-buffer ()
-  "Install a package from the current buffer.
+  "Install package from current buffer.
 The current buffer is assumed to be a single .el or .tar file or
 a directory.  These must follow the packaging guidelines (see
 info node `(elisp)Packaging').
@@ -2081,7 +2104,7 @@ package-install-from-buffer
 
 ;;;###autoload
 (defun package-install-file (file)
-  "Install a package from a file.
+  "Install package from FILE.
 The file can either be a tar file, an Emacs Lisp file, or a
 directory."
   (interactive "fPackage file name: ")
@@ -2217,7 +2240,7 @@ package-reinstall
 
 ;;;###autoload
 (defun package-autoremove ()
-  "Remove packages that are no more needed.
+  "Remove packages that are no longer needed.
 
 Packages that are no more needed by other packages in
 `package-selected-packages' and their dependencies
@@ -2334,6 +2357,8 @@ package--get-description
      )))
 
 (defun describe-package-1 (pkg)
+  "Insert package description of PKG at point.
+Helper function for `describe-package'."
   (require 'lisp-mnt)
   (let* ((desc (or
                 (if (package-desc-p pkg) pkg)
@@ -2563,6 +2588,9 @@ describe-package-1
       (browse-url-add-buttons))))
 
 (defun package-install-button-action (button)
+  "Run `package-install' on package defined by BUTTON.
+Used for the 'action property of buttons in the buffer created by
+`describe-package'."
   (let ((pkg-desc (button-get button 'package-desc)))
     (when (y-or-n-p (format-message "Install package `%s'? "
                                     (package-desc-full-name pkg-desc)))
@@ -2571,6 +2599,9 @@ package-install-button-action
       (goto-char (point-min)))))
 
 (defun package-delete-button-action (button)
+  "Run `package-delete' on package defined by BUTTON.
+Used for the 'action property of buttons in the buffer created by
+`describe-package'."
   (let ((pkg-desc (button-get button 'package-desc)))
     (when (y-or-n-p (format-message "Delete package `%s'? "
                                     (package-desc-full-name pkg-desc)))
@@ -2579,10 +2610,16 @@ package-delete-button-action
       (goto-char (point-min)))))
 
 (defun package-keyword-button-action (button)
+  "Show *Packages* buffer filtered by keyword from BUTTON label.
+Used for the 'action property of buttons in the buffer created by
+`describe-package'."
   (let ((pkg-keyword (button-get button 'package-keyword)))
     (package-show-package-list t (list pkg-keyword))))
 
-(defun package-make-button (text &rest props)
+(defun package-make-button (text &rest properties)
+  "Insert button labelled TEXT with button PROPERTIES at point.
+PROPERTIES are passed to `insert-text-button', for which this
+function is a convenience wrapper used by `describe-package-1'."
   (let ((button-text (if (display-graphic-p) text (concat "[" text "]")))
         (button-face (if (display-graphic-p)
                          '(:box (:line-width 2 :color "dark grey")
@@ -2590,9 +2627,12 @@ package-make-button
                                 :foreground "black")
                        'link)))
     (apply #'insert-text-button button-text 'face button-face 'follow-link t
-           props)))
+           properties)))
 
 (defun package--print-email-button (name)
+  "Insert a button to email NAME at point.
+NAME should have the form (FULLNAME . EMAIL) where NAME is either
+a full name or nil, and EMAIL is a valid email address."
   (when (car name)
     (insert (car name)))
   (when (and (car name) (cdr name))
@@ -2701,13 +2741,13 @@ package--push
      (push (cons ,pkg-desc ,status) ,listname)))
 
 (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'.")
 
 (defvar package-list-unsigned nil
   "If non-nil, mention in the list which packages were installed w/o signature.")
 
 (defvar package--emacs-version-list (version-to-list emacs-version)
-  "`emacs-version', as a list.")
+  "Variable `emacs-version' as a list.")
 
 (defun package--incompatible-p (pkg &optional shallow)
   "Return non-nil if PKG has no chance of being installable.
@@ -2782,7 +2822,7 @@ package-menu--hide-packages
 Installed obsolete packages are always displayed.")
 
 (defun package-menu-toggle-hiding ()
-  "Toggle visibility of obsolete available packages."
+  "In Package Menu, toggle visibility of obsolete available packages."
   (interactive)
   (unless (derived-mode-p 'package-menu-mode)
     (user-error "The current buffer is not a Package Menu"))
@@ -2840,7 +2880,7 @@ package-hidden-regexps
 omitted from the package menu.  To toggle this, type \\[package-menu-toggle-hiding].
 
 Values can be interactively added to this list by typing
-\\[package-menu-hide-package] on a package"
+\\[package-menu-hide-package] on a package."
   :version "25.1"
   :type '(repeat (regexp :tag "Hide packages with name matching")))
 
@@ -3100,7 +3140,7 @@ package-menu-refresh
   (package-refresh-contents package-menu-async))
 
 (defun package-menu-hide-package ()
-  "Hide a package under point.
+  "Hide a package under point in Package Menu.
 If optional arg BUTTON is non-nil, describe its associated package."
   (interactive)
   (declare (interactive-only "change `package-hidden-regexps' instead."))
@@ -3199,6 +3239,7 @@ package-menu-quick-help
   'package-menu-view-commentary 'package-menu-describe-package "24.1")
 
 (defun package-menu-get-status ()
+  "Return status text of package at point in Package Menu."
   (let* ((id (tabulated-list-get-id))
          (entry (and id (assoc id tabulated-list-entries))))
     (if entry
@@ -3224,6 +3265,10 @@ package-desc-priority-version
         (package-desc-version pkg-desc)))
 
 (defun package-menu--find-upgrades ()
+  "In Package Menu, return an alist of packages that can be upgraded.
+The alist has the same form as `package-alist', namely a list
+of (PKG . DESCS), but where DESCS is the `package-desc' object
+corresponding to the newer version."
   (let (installed available upgrades)
     ;; Build list of installed/available packages in this buffer.
     (dolist (entry tabulated-list-entries)
@@ -3487,7 +3532,7 @@ package-menu--find-and-notify-upgrades
 
 
 (defun package-menu--post-refresh ()
-  "If there's a *Packages* buffer, revert it and check for new packages and upgrades.
+  "Revert *Packages* buffer and check for new packages and upgrades.
 Do nothing if there's no *Packages* buffer.
 
 This function is called after `package-refresh-contents' and it
-- 
2.20.1

bug#37410: [PATCH] Several doc fixes in package.el

[Eli Zaretskii]

> From: Stefan Kangas <stefan@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."

bug#37410: [PATCH] Several doc fixes in package.el

[Stefan Kangas]

[-- Attachment #1: Type: text/plain, Size: 3104 bytes --]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Stefan Kangas <stefan@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

[-- Attachment #2: 0001-Several-doc-fixes-in-package.el.patch --]
[-- Type: text/x-patch, Size: 13450 bytes --]

From baceba6363e9a6ed09cd2aa878e15c090616dd09 Mon Sep 17 00:00:00 2001
From: Stefan Kangas <stefankangas@gmail.com>
Date: Sun, 15 Sep 2019 17:41:27 +0200
Subject: [PATCH] Several doc fixes in package.el

* lisp/emacs-lisp/package.el (top-level)
(package-check-signature, package--from-builtin)
(package-desc-full-name, package-desc-suffix)
(package-desc--keywords, package--bi-desc)
(package-process-define-package, package-archive-base)
(package-install-from-archive, package-install-from-buffer)
(package-install-file, package-autoremove, describe-package-1)
(package-install-button-action, package-delete-button-action)
(package-keyword-button-action, package-make-button)
(package--print-email-button, package-list-unversioned)
(package--emacs-version-list, package-menu-toggle-hiding)
(package-hidden-regexps, package-menu-hide-package)
(package-menu-get-status, package-menu--find-upgrades)
(package-menu--post-refresh): Doc fixes.  (Bug#37410)
---
 lisp/emacs-lisp/package.el | 103 +++++++++++++++++++++++++++----------
 1 file changed, 76 insertions(+), 27 deletions(-)

diff --git a/lisp/emacs-lisp/package.el b/lisp/emacs-lisp/package.el
index ef0c5171de..18b5c962f6 100644
--- a/lisp/emacs-lisp/package.el
+++ b/lisp/emacs-lisp/package.el
@@ -120,9 +120,9 @@
 ;; - "installed" instead of a blank in the status column
 ;; - tramp needs its files to be compiled in a certain order.
 ;;   how to handle this?  fix tramp?
-;; - maybe we need separate .elc directories for various emacs versions
-;;   and also emacs-vs-xemacs.  That way conditional compilation can
-;;   work.  But would this break anything?
+;; - maybe we need separate .elc directories for various emacs
+;;   versions.  That way conditional compilation can work.  But would
+;;   this break anything?
 ;; - William Xu suggests being able to open a package file without
 ;;   installing it
 ;; - Interface with desktop.el so that restarting after an install
@@ -354,9 +354,9 @@ package-check-signature
 
 (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 so, and variable `package-check-signature' is
+`allow-unsigned', return `allow-unsigned', otherwise return the
+value of variable `package-check-signature'."
   (if (eq package-check-signature 'allow-unsigned)
       (progn
         (require 'epg-config)
@@ -472,6 +472,8 @@ package--default-summary
   signed)
 
 (defun package--from-builtin (bi-desc)
+  "Create a `package-desc' object from BI-DESC.
+BI-DESC should be a `package--bi-desc' object."
   (package-desc-create :name (pop bi-desc)
                        :version (package--bi-desc-version bi-desc)
                        :summary (package--bi-desc-summary bi-desc)
@@ -509,11 +511,21 @@ package-version-join
       (apply #'concat (nreverse str-list)))))
 
 (defun package-desc-full-name (pkg-desc)
+  "Return full name of package-desc object PKG-DESC.
+This is the name of the package with its version appended."
   (format "%s-%s"
           (package-desc-name pkg-desc)
           (package-version-join (package-desc-version pkg-desc))))
 
 (defun package-desc-suffix (pkg-desc)
+  "Return file-name extension of package-desc object PKG-DESC.
+Depending on the `package-desc-kind' of PKG-DESC, this is one of:
+
+   'single - \".el\"
+   'tar    - \".tar\"
+   'dir    - \"\"
+
+If the kind is none of the above, signal an error."
   (pcase (package-desc-kind pkg-desc)
     ('single ".el")
     ('tar ".tar")
@@ -521,6 +533,10 @@ package-desc-suffix
     (kind (error "Unknown package kind: %s" kind))))
 
 (defun package-desc--keywords (pkg-desc)
+  "Return keywords of package-desc object PKG-DESC.
+These keywords come from the foo-pkg.el file, and in general
+corresponds to the keywords in the \"Keywords\" header of the
+package."
   (let ((keywords (cdr (assoc :keywords (package-desc-extras pkg-desc)))))
     (if (eq (car-safe keywords) 'quote)
         (nth 1 keywords)
@@ -530,10 +546,10 @@ package-desc-priority
   "Return the priority of the archive of package-desc object P."
   (package-archive-priority (package-desc-archive p)))
 
-;; Package descriptor format used in finder-inf.el and package--builtins.
 (cl-defstruct (package--bi-desc
                (:constructor package-make-builtin (version summary))
                (:type vector))
+  "Package descriptor format used in finder-inf.el and package--builtins."
   version
   reqs
   summary)
@@ -575,7 +591,15 @@ package-activated-list
 ;; The following functions are called on each installed package by
 ;; `package-load-all-descriptors', which ultimately populates the
 ;; `package-alist' variable.
+
 (defun package-process-define-package (exp)
+  "Process define-package expression EXP and push it to `package-alist'.
+EXP should be a form read from a foo-pkg.el file.
+Convert EXP into a `package-desc' object using the
+`package-desc-from-define' constructor before pushing it to
+`package-alist'.
+If there already exists a package by that name in
+`package-alist', replace that definition with the new one."
   (when (eq (car-safe exp) 'define-package)
     (let* ((new-pkg-desc (apply #'package-desc-from-define (cdr exp)))
            (name (package-desc-name new-pkg-desc))
@@ -866,6 +890,7 @@ package--alist-to-plist-args
   (mapcar #'macroexp-quote
           (apply #'nconc
                  (mapcar (lambda (pair) (list (car pair) (cdr pair))) alist))))
+
 (defun package-unpack (pkg-desc)
   "Install the contents of the current buffer as a package."
   (let* ((name (package-desc-name pkg-desc))
@@ -1899,12 +1924,13 @@ package--sort-by-dependence
 ;; installed in a variety of ways (archives, buffer, file), but
 ;; requirements (dependencies) are always satisfied by looking in
 ;; `package-archive-contents'.
+
 (defun package-archive-base (desc)
-  "Return the archive containing the package NAME."
+  "Return the package described by DESC."
   (cdr (assoc (package-desc-archive desc) package-archives)))
 
 (defun package-install-from-archive (pkg-desc)
-  "Download and install a tar package."
+  "Download and install a tar package defined by PKG-DESC."
   ;; This won't happen, unless the archive is doing something wrong.
   (when (eq (package-desc-kind pkg-desc) 'dir)
     (error "Can't install directory package from archive"))
@@ -2081,7 +2107,7 @@ package-install-from-buffer
 
 ;;;###autoload
 (defun package-install-file (file)
-  "Install a package from a file.
+  "Install package from FILE.
 The file can either be a tar file, an Emacs Lisp file, or a
 directory."
   (interactive "fPackage file name: ")
@@ -2217,7 +2243,7 @@ package-reinstall
 
 ;;;###autoload
 (defun package-autoremove ()
-  "Remove packages that are no more needed.
+  "Remove packages that are no longer needed.
 
 Packages that are no more needed by other packages in
 `package-selected-packages' and their dependencies
@@ -2334,6 +2360,8 @@ package--get-description
      )))
 
 (defun describe-package-1 (pkg)
+  "Insert the package description for PKG.
+Helper function for `describe-package'."
   (require 'lisp-mnt)
   (let* ((desc (or
                 (if (package-desc-p pkg) pkg)
@@ -2563,6 +2591,9 @@ describe-package-1
       (browse-url-add-buttons))))
 
 (defun package-install-button-action (button)
+  "Run `package-install' on the package BUTTON points to.
+Used for the 'action property of buttons in the buffer created by
+`describe-package'."
   (let ((pkg-desc (button-get button 'package-desc)))
     (when (y-or-n-p (format-message "Install package `%s'? "
                                     (package-desc-full-name pkg-desc)))
@@ -2571,6 +2602,9 @@ package-install-button-action
       (goto-char (point-min)))))
 
 (defun package-delete-button-action (button)
+  "Run `package-delete' on the package BUTTON points to.
+Used for the 'action property of buttons in the buffer created by
+`describe-package'."
   (let ((pkg-desc (button-get button 'package-desc)))
     (when (y-or-n-p (format-message "Delete package `%s'? "
                                     (package-desc-full-name pkg-desc)))
@@ -2579,10 +2613,17 @@ package-delete-button-action
       (goto-char (point-min)))))
 
 (defun package-keyword-button-action (button)
+  "Show filtered \"*Packages*\" buffer for BUTTON.
+The buffer is filtered by the `package-keyword' property of BUTTON.
+Used for the 'action property of buttons in the buffer created by
+`describe-package'."
   (let ((pkg-keyword (button-get button 'package-keyword)))
     (package-show-package-list t (list pkg-keyword))))
 
-(defun package-make-button (text &rest props)
+(defun package-make-button (text &rest properties)
+  "Insert button labeled TEXT with button PROPERTIES at point.
+PROPERTIES are passed to `insert-text-button', for which this
+function is a convenience wrapper used by `describe-package-1'."
   (let ((button-text (if (display-graphic-p) text (concat "[" text "]")))
         (button-face (if (display-graphic-p)
                          '(:box (:line-width 2 :color "dark grey")
@@ -2590,20 +2631,23 @@ package-make-button
                                 :foreground "black")
                        'link)))
     (apply #'insert-text-button button-text 'face button-face 'follow-link t
-           props)))
-
-(defun package--print-email-button (name)
-  (when (car name)
-    (insert (car name)))
-  (when (and (car name) (cdr name))
+           properties)))
+
+(defun package--print-email-button (recipient)
+  "Insert a button whose action will send an email to RECIPIENT.
+NAME should have the form (FULLNAME . EMAIL) where FULLNAME is
+either a full name or nil, and EMAIL is a valid email address."
+  (when (car recipient)
+    (insert (car recipient)))
+  (when (and (car recipient) (cdr recipient))
     (insert " "))
-  (when (cdr name)
+  (when (cdr recipient)
     (insert "<")
-    (insert-text-button (cdr name)
+    (insert-text-button (cdr recipient)
                         'follow-link t
                         'action (lambda (_)
                                   (compose-mail
-                                   (format "%s <%s>" (car name) (cdr name)))))
+                                   (format "%s <%s>" (car recipient) (cdr recipient)))))
     (insert ">"))
   (insert "\n"))
 
@@ -2701,13 +2745,13 @@ package--push
      (push (cons ,pkg-desc ,status) ,listname)))
 
 (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-packages'.")
 
 (defvar package-list-unsigned nil
   "If non-nil, mention in the list which packages were installed w/o signature.")
 
 (defvar package--emacs-version-list (version-to-list emacs-version)
-  "`emacs-version', as a list.")
+  "The value of variable `emacs-version' as a list.")
 
 (defun package--incompatible-p (pkg &optional shallow)
   "Return non-nil if PKG has no chance of being installable.
@@ -2782,7 +2826,7 @@ package-menu--hide-packages
 Installed obsolete packages are always displayed.")
 
 (defun package-menu-toggle-hiding ()
-  "Toggle visibility of obsolete available packages."
+  "In Package Menu, toggle visibility of obsolete available packages."
   (interactive)
   (unless (derived-mode-p 'package-menu-mode)
     (user-error "The current buffer is not a Package Menu"))
@@ -2840,7 +2884,7 @@ package-hidden-regexps
 omitted from the package menu.  To toggle this, type \\[package-menu-toggle-hiding].
 
 Values can be interactively added to this list by typing
-\\[package-menu-hide-package] on a package"
+\\[package-menu-hide-package] on a package."
   :version "25.1"
   :type '(repeat (regexp :tag "Hide packages with name matching")))
 
@@ -3100,7 +3144,7 @@ package-menu-refresh
   (package-refresh-contents package-menu-async))
 
 (defun package-menu-hide-package ()
-  "Hide a package under point.
+  "Hide a package under point in Package Menu.
 If optional arg BUTTON is non-nil, describe its associated package."
   (interactive)
   (declare (interactive-only "change `package-hidden-regexps' instead."))
@@ -3199,6 +3243,7 @@ package-menu-quick-help
   'package-menu-view-commentary 'package-menu-describe-package "24.1")
 
 (defun package-menu-get-status ()
+  "Return status text of package at point in Package Menu."
   (let* ((id (tabulated-list-get-id))
          (entry (and id (assoc id tabulated-list-entries))))
     (if entry
@@ -3224,6 +3269,10 @@ package-desc-priority-version
         (package-desc-version pkg-desc)))
 
 (defun package-menu--find-upgrades ()
+  "In Package Menu, return an alist of packages that can be upgraded.
+The alist has the same form as `package-alist', namely a list
+of (PKG . DESCS), but where DESCS is the `package-desc' object
+corresponding to the newer version."
   (let (installed available upgrades)
     ;; Build list of installed/available packages in this buffer.
     (dolist (entry tabulated-list-entries)
@@ -3487,7 +3536,7 @@ package-menu--find-and-notify-upgrades
 
 
 (defun package-menu--post-refresh ()
-  "If there's a *Packages* buffer, revert it and check for new packages and upgrades.
+  "Revert \"*Packages*\" buffer and check for new packages and upgrades.
 Do nothing if there's no *Packages* buffer.
 
 This function is called after `package-refresh-contents' and it
-- 
2.20.1

bug#37410: [PATCH] Several doc fixes in package.el

[Stefan Kangas]

Stefan Kangas <stefan@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