bug#60418: [PATCH] Add :vc keyword to use-package

[Eli Zaretskii]

> Cc: 60418@debbugs.gnu.org, Felician Nemeth <felician.nemeth@gmail.com>,
>  stefankangas@gmail.com
> Date: Sun, 16 Apr 2023 17:43:02 +0200
> From:  Tony Zorman via "Bug reports for GNU Emacs,
>  the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org>
> 
> Alright, attached are patches that contain the requested change: we now
> default to :last-release.

Thanks, please see below a few minor comments.

> +(defun use-package-vc-install (arg &optional local-path)
> +  "ARG is a list of the form (NAME OPTIONS REVISION).

This should tell what the function does, not just what the arguments
look like.

> +The optional LOCAL-PATH boolean decides whether
> +`package-vc-install-from-checkout' or `package-vc-install' will
> +end up being called."

This should tell explicitly which of the two is called when
LOCAL-PATH is nil and when it's non-nil.

> +(defun use-package-handler/:vc (name _keyword arg rest state)
> +  "Generate code for the :vc keyword."

I don't think this is an accurate description of what the function
does.  Also, we try very hard to mention at least the mandatory
arguments in the first line of the doc strings.

> @@ -1666,7 +1744,8 @@ use-package
>                   (compare with `custom-set-variables').
>  :custom-face     Call `custom-set-faces' with each face definition.
>  :ensure          Loads the package using package.el if necessary.
> -:pin             Pin the package to an archive."
> +:pin             Pin the package to an archive.
> +:vc              Integration with `package-vc.el'."

The description of other keywords say what is the effect of each one;
the description of :vc doesn't.  "Integration with package.el" is not
a useful description, it says nothing about what this keyword does.

> +@findex :vc
> +The @code{:vc} keyword can be used to control how packages are fetched.

Without saying more regarding what "fetched" is about, this
description is not as useful as it could have been.  You should give
some context which would explain how "fetching" is related to
use-package.

My suggestion, btw, is to use a more descriptive "downloading", not
"fetching".

> +It accepts the same arguments as @code{package-vc-selected-packages},

There should be a cross-reference here to the Emacs manual where it
describes package-vc-selected-packages.

> +except that a name need not explicitly given: it is inferred from the
                              ^
"be" is missing there.

> +declaration.  Further, the accepted property list is augmented by a
> +@code{:rev} keyword, which has the same shape as the @code{REV} argument
> +to @code{package-vc-install}.  Notably—even when not specified—@code{:rev}
                                         ^
Please don't use non-ASCII characters in Texinfo sources, that is
usually unnecessary.  In this case, to produce an em-dash, use two
dashes in a row -- they will be converted to em-dash on output.

> +would try—by invoking @code{package-vc-install}—to install the latest

Same here.

> +The above dispatches to @code{package-vc-install-from-checkout}.

A cross-reference here would be beneficial, again.

> +** use-package
> +
> +*** New ':vc' keyword

Heading lines in NEWS should end in a period.

Also, this entry should be marked with "+++", as the necessary changes
in the manuals are included.

> +This keyword enables the user to control how packages are fetched by
> +utilising 'package-vc.el'.  By default, it relays its arguments to
> +'package-vc-install', but—when combined with the ':load-path'
> +keyword—it can also call upon 'package-vc-install-from-checkout'
> +instead.

Please also avoid non-ASCII characters in NEWS.

>           Further, if no revision is given via the ':rev' argument, we
> +fall back to the last release (via 'package-vc-install's
> +':last-release' argument).  To check out the last commit, use ':rev

You seem to like to say "Further," at the beginning of sentences, but
please be aware that this word usually adds no useful information, so
you can easily drop it in most cases.  For example, in the text above:
the sentence will be as informative without "Further" as with it.

Also, "we fall back" is not our style in documentation (who is "we"?).
We say "use-package will fall back" instead.

Thanks for working on this.