Eli Zaretskii writes: >> Cc: Philip Kaludercic >> 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" >> >> 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: > Thank you, I've attached an updated patch below. >> @@ -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. > Sure, done. >> +`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. > Done. >> (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'. > I went with a variation of your phrasing that I think is a little more precise. LMK what you think. >> + >> +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? > It's just for indicating that we're talking about VC packages specifically. Does that make sense? >> +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. > Yes, this patch only updates the documentation, but I agree that this behavior may deserve another look as well. >> (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? > I agree. Philip, any objections to renaming this variable? I don't know if people already use it. >> + 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." > Done. >> (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. > Alright, I went with DEPS for brevity. >> +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. > Updated. >> (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." > Done. >> (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.) > Updated. >> -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. > Sure, done. >> +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. > Good catch, thanks. I've removed the "is" in the updated patch. >> (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". Alright, I made a couple more changes to this one.