bug#65386: [PATCH] ; Refine some 'package-vc' docstrings

[Eli Zaretskii]

> Cc: Philip Kaludercic <philipk@posteo.net>
> Date: Sat, 19 Aug 2023 20:07:07 +0200
> From:  Eshel Yaron via "Bug reports for GNU Emacs,
>  the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org>
> 
> This patch includes improvement suggestions for the docstrings of some
> of the functions and user options in package-vc.el.

Thanks.  Allow me a few comments:

> @@ -94,7 +94,10 @@ package-vc-heuristic-alist
>                   (+ (or alnum "-" "." "_")) (? "/")))
>            eos)
>       . Bzr))
> -  "Heuristic mapping URL regular expressions to VC backends."
> +  "Alist mapping regular expressions to VC backends.

The first line loses information by omitting the "URL" part, which I
think is important, more important than the "regular expressions"
part.  How about

  Alist mapping repository URLs to VC backends.

> +`package-vc-install' tries to match each regular expression in
> +this alist to the repository URL you give it, and uses the VC
> +backend corresponding to the first matching regular expression."

This tends to describe the implementation, not the purpose.  I would
rephrase

  `package-vc-install' consults this alist to determine the VC backend
  from the repository URL.  Each element of the alist has the form
  (URL-REGEXP . BACKEND).  `package-vc-install' will use BACKEND of
  the first association for which the URL of the repository matches
  the URL-REGEXP of the association.

>  (defcustom package-vc-default-backend 'Git
> -  "Default VC backend used when cloning a package repository.
> -If no repository type was specified or could be guessed by
> -`package-vc-heuristic-alist', this is the default VC backend
> -used as fallback.  The value must be a member of
> -`vc-handled-backends' and the named backend must implement
> -the `clone' function."
> -  :type `(choice ,@(mapcar (lambda (b) (list 'const b))
> -                           vc-handled-backends))
> +  "Default VC backend to use for cloning package repositories.
> +`package-vc-install' uses this backend when you don't specify a
> +backend and give it a URL that doesn't match any regular
> +expression in `package-vc-heuristic-alist'.

I guess you mean

  `package-vc-install' uses this backend when you specify neither the
  backend nor a repository URL to map via `package-vc-heuristic-alist'.

> +
> +The value must be a member of `vc-handled-backends' that supports
> +the `clone' VC function."
> +  :type `(choice ,@(seq-keep
> +                    (lambda (be)
> +                      (when (or (vc-find-backend-function be 'clone)
> +                                (alist-get 'clone (get be 'vc-functions)))
> +                        (list 'const be)))
> +                    vc-handled-backends))
>    :version "29.1")
>  
>  (defcustom package-vc-register-as-project t
> @@ -140,20 +148,21 @@ package-vc-install-selected-packages
>                 (package-desc-create :name name :kind 'vc))
>             spec)))))))
>  
> -(defcustom package-vc-selected-packages '()
> -  "List of packages that must be installed.
> -Each member of the list is of the form (NAME . SPEC), where NAME
> -is a symbol designating the package and SPEC is one of:
> +
> +(defcustom package-vc-selected-packages nil
> +  "List of packages to install from source.

What is the significance of "from source" here?

> +The command `package-vc-install' updates the value of this user
> +option to store package specifications for packages that are not
> +specified in any archive."

I question the wisdom of having commands modify values of user
options.  I think it is better to have a separate variable whose
initial value is taken from the user option, and then have the
commands add to that separate variable, leaving the user option
intact.

>  (defcustom package-vc-allow-side-effects nil
> -  "Whether to process :make and :shell-command spec arguments.
> +  "Whether to run extra build commands when installing VC packages.

The name of this variable is very far from what it means.  Running
commands and "side effects" are barely related to each other.  Please
consider renaming the variable.  How about
package-vc-allow-build-commands?

> +                                                          When
> +this option is nil, Emacs ignores these keywords when installing
> +and upgrading VC packages.  If it's a list of package
> +names (symbols), Emacs only runs these extra build commands for
> +those packages.

"By default, Emacs ignores these keywords [...], but if the value is a
list of package names (symbols), the build commands will be run for
those packages."

>  (defun package-vc-install-dependencies (requirements)
> -  "Install missing dependencies, and return missing ones.
> -The return value will be nil if everything was found, or a list
> -of (NAME VERSION) pairs of all packages that couldn't be found.
> -
> -REQUIREMENTS should be a list of additional requirements; each
> -element in this list should have the form (PACKAGE VERSION-LIST),
> -where PACKAGE is a package name and VERSION-LIST is the required
> -version of that package."
> +  "Install missing dependencies according to REQUIREMENTS.
> +
> +REQUIREMENTS is a list of elements (PACKAGE VERSION-LIST), where
> +PACKAGE is a package name and VERSION-LIST is the required
> +version of that package.

This seems to indicate that REQUIREMENTS should be renamed to
DEPENDENCIES.

> +Return a list of requirements that couldn't be met (or nil, when
> +this function successfully installs all given requirements)."
                              ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
"Installs requirements"? that's why "dependencies" would be better.

>  (defun package-vc-upgrade (pkg-desc)
> -  "Attempt to upgrade the package PKG-DESC."
> +  "Upgrade the VC package that PKG-DESC describes."

"Upgrade the package described by PKG-DESC from the package's VC repository."

>  (defun package-vc-install (package &optional rev backend name)
>    "Fetch a PACKAGE and set it up for using with Emacs.

"Fetch a package described by PACKAGE and set it up for use with Emacs."

(The point being that PACKAGE is not just a name, it's a description
that can have several forms.)

> -If PACKAGE is a string containing an URL, download the package
> -from the repository at that URL; the function will try to guess
> -the name of the package from the URL.  This can be overridden by
> -passing the optional argument NAME.  If PACKAGE is a cons-cell,
> -it should have the form (NAME . SPEC), where NAME is a symbol
> -indicating the package name and SPEC is a plist as described in
> -`package-vc-selected-packages'.  Otherwise PACKAGE should be a
> -symbol whose name is the package name, and the URL for the
> -package will be taken from the package's metadata.
> +PACKAGE specifies which package to install, where to find its
> +source repository and how to build it.
>  
>  By default, this function installs the last revision of the
>  package available from its repository.  If REV is a string, it
> -describes the revision to install, as interpreted by the VC
> -backend.  The special value `:last-release' (interactively, the
> -prefix argument), will use the commit of the latest release, if
> -it exists.  The last release is the latest revision which changed
> -the \"Version:\" header of the package's main Lisp file.
> -
> -Optional argument BACKEND specifies the VC backend to use for cloning
> -the package's repository; this is only possible if NAME-OR-URL is a URL,
> -a string.  If BACKEND is omitted or nil, the function
> -uses `package-vc-heuristic-alist' to guess the backend.
> -Note that by default, a VC package will be prioritized over a
> -regular package, but it will not remove a VC package.
> -
> -\(fn PACKAGE &optional REV BACKEND)"
> +describes the revision to install, as interpreted by the relevant
> +VC backend.  The special value `:last-release' (interactively,
> +the prefix argument), says to use the commit of the latest
> +release, if it exists.  The last release is the latest revision
> +which changed the \"Version:\" header of the package's main Lisp
> +file.
> +
> +If PACKAGE is a symbol, install the package with that name
> +according to metadata that package archives provide for it.  This
> +is the simplest way to call this function, but it only works if
> +the package you want to install is listed in a package archive
> +you have configured.
> +
> +If PACKAGE is a string, it specifies the URL of the package
> +repository.  In this case, optional argument BACKEND specifies
> +the VC backend to use for cloning the repository; if it's nil,
> +this function tries to infer which backend to use according to
> +the value of `package-vc-heuristic-alist' and if that fails it
> +uses `package-vc-default-backend'.  Optional argument NAME
> +specifies the package name in this case; if it's nil, this
> +package uses `file-name-base' on the URL to obtain the package
> +name, otherwise NAME is the package name as a symbol.
> +
> +PACKAGE can also be a cons cell (PNAME . SPEC) where PNAME is the
> +package name as a symbol, and SPEC is a plist that specifes how
> +to fetch and build the package.  For possible values, see the
> +subsection \"Specifying Package Sources\" in the Info
> +node `(emacs)Fetching Package Sources'.

I think the doc string should describe PACKAGE before REV.

> +If you use this function to install a package that you also have
> +installed from a package archive, the version this function
> +installs is takes precedence."

"is takes"? something's wrong here.

>  (defun package-vc-prepare-patch (pkg-desc subject revisions)
> -  "Send patch for REVISIONS to maintainer of the package PKG using SUBJECT.
> +  "Send patch for REVISIONS to maintainer of the package PKG-DESC using 
> SUBJECT.

This should mention email, otherwise SUBJECT is confusing.  Maybe use
"Email" instead of "Send".