Subject: bug#37410: closed (Re: bug#37410: [PATCH] Several doc fixes in package.el)

From: [email-masked] (GNU bug Tracking System)
Date: Sat, 21 Sep 2019 21:15:02 +0000

Your bug report

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

which was filed against the emacs package, has been closed.

The explanation is attached below, along with your original report.
If you require more details, please reply to [email-masked].

-- 
37410: http://debbugs.gnu.org/cgi/bugreport.cgi?bug=37410
GNU Bug Tracking System
Contact [email-masked] with problems

Subject: Re: bug#37410: [PATCH] Several doc fixes in package.el
From: Stefan Kangas <[email-masked]>
To: Eli Zaretskii <[email-masked]>
Cc: [email-masked]
Date: Sat, 21 Sep 2019 23:14:35 +0200
List-Id: <debbugs-submit.debbugs.gnu.org>

Stefan Kangas <[email-masked]> 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

Subject: [PATCH] Several doc fixes in package.el
From: Stefan Kangas <[email-masked]>
To: [email-masked]
Date: Sun, 15 Sep 2019 18:01:17 +0200
List-Id: <debbugs-submit.debbugs.gnu.org>

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
From a5c324028dd001a3a759534b99866afa05a2535b Mon Sep 17 00:00:00 2001
From: Stefan Kangas <[email-masked]>
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