[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
bug#65386: [PATCH] ; Refine some 'package-vc' docstrings
From: |
Eshel Yaron |
Subject: |
bug#65386: [PATCH] ; Refine some 'package-vc' docstrings |
Date: |
Sat, 19 Aug 2023 22:42:57 +0200 |
User-agent: |
Gnus/5.13 (Gnus v5.13) |
Eli Zaretskii <eliz@gnu.org> writes:
>> 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:
>
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.
v2-0001-Refine-some-package-vc-docstrings.patch
Description: Text Data
- bug#65386: [PATCH] ; Refine some 'package-vc' docstrings, Eshel Yaron, 2023/08/19
- bug#65386: [PATCH] ; Refine some 'package-vc' docstrings, Eli Zaretskii, 2023/08/19
- bug#65386: [PATCH] ; Refine some 'package-vc' docstrings,
Eshel Yaron <=
- bug#65386: [PATCH] ; Refine some 'package-vc' docstrings, Eli Zaretskii, 2023/08/20
- bug#65386: [PATCH] ; Refine some 'package-vc' docstrings, Eshel Yaron, 2023/08/20
- bug#65386: [PATCH] ; Refine some 'package-vc' docstrings, Philip Kaludercic, 2023/08/20
- bug#65386: [PATCH] ; Refine some 'package-vc' docstrings, Eshel Yaron, 2023/08/20
- bug#65386: [PATCH] ; Refine some 'package-vc' docstrings, Philip Kaludercic, 2023/08/20
- bug#65386: [PATCH] ; Refine some 'package-vc' docstrings, Eshel Yaron, 2023/08/20
- bug#65386: [PATCH] ; Refine some 'package-vc' docstrings, Philip Kaludercic, 2023/08/20
- bug#65386: [PATCH] ; Refine some 'package-vc' docstrings, Eshel Yaron, 2023/08/20
- bug#65386: [PATCH] ; Refine some 'package-vc' docstrings, Philip Kaludercic, 2023/08/20
- bug#65386: [PATCH] ; Refine some 'package-vc' docstrings, Eshel Yaron, 2023/08/20