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

[Philip Kaludercic]

Eshel Yaron <me@eshelyaron.com> writes:

> Thanks for reviewing, I've replied to your comments and attached an
> updated patch (v4) below.
>
> Philip Kaludercic <philipk@posteo.net> writes:
>
>>>    :type `(alist :key-type (regexp :tag "Regular expression matching URLs")
>>>                  :value-type (choice :tag "VC Backend"
>>>                                      ,@(mapcar (lambda (b) `(const ,b))
>>> @@ -102,14 +107,19 @@ package-vc-heuristic-alist
>>>    :version "29.1")
>>>
>>>  (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 specify neither
>>> +the backend nor a repository URL that's recognized 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))
>>
>> This is good, but shouldn't we do something like this in a separate
>> patch?  No hard opinions on that though.
>
> I thought it was a small enough change to include along with the doc
> improvements, but if you prefer I can update the `:type` in a separate
> patch.  BTW, this should probably be applied also to
> `package-vc-heuristic-alist` either way.

In that case I think it would be better to prepare a separate patch.
Also, would it make sense to determine this at compile-time?  On the
other hand, if a VC backend is installed later on from ELPA, we would
want the custom type to reflect this.

>> Also, it seems this is not returning all the available VC backends...
>
> Which backend do you see missing?  It should include all VC backends
> that support `vc-call-backend` with `clone` as FUNCTION-NAME.  I get
> SVN, Bzr, Git and Hg.  Anything missing?  Is there a better way to check
> if a VC backend supports cloning for the purposes of `package-vc`?

Nevermind, I was mistaken in believing there was a vc-cvs-clone
implementation... but for some reason I appear not to be able to find
any reference to the implementation except in a ChangeLog file...
>>> +Some packages specify \"make\" targets or other shell commands
>>> +that should run prior to building the package, by including the
>>> +:make or :shell-command keywords in their specification.  By
>>> +default, Emacs ignores these keywords when installing and
>>> +upgrading VC packages, but if the value is a list of package
>>> +names (symbols), the build commands will be run for those
>>
>> Perhaps "... as symbols"?
>>
>
> I tried that but I find it makes the sentence less comprehensible
> because it's already quite long, and the parenthesis help with that a
> bit.  But I'm fine with either phrasing if you feel strongly about it.

No strong opinions here.

>>>  ;;;###autoload
>>>  (defun package-vc-upgrade-all ()
>>> -  "Attempt to upgrade all installed VC packages."
>>> +  "Upgrade all installed VC packages."
>>
>> The attempt here is intentional, since upgrading can fail if it was not
>> possible to successfully update the local VCS state due to merge
>> conflicts.  We can mention that there, and emphasise it at other places
>> as a major downside of VC packages.
>>
>
> Got it.  I don't think it's that useful to include "attempt to" in the
> first line, because most commands have some failure modes.  Instead,
> I've added a sentence about this possible failure after the first line.

Fine with that.

>>>    (interactive (list (package-vc--read-package-desc "Upgrade VC package: " 
>>> t)))
>>>    ;; HACK: To run `package-vc--unpack-1' after checking out the new
>>>    ;; revision, we insert a hook into `vc-post-command-functions', and
>>> @@ -792,34 +810,45 @@ package-vc--release-rev
>>>
>>>  ;;;###autoload
>>>  (defun package-vc-install (package &optional rev backend name)
>>> -  "Fetch a PACKAGE and set it up for using with Emacs.
>>> -
>>> -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.
>>> +  "Fetch a package described by PACKAGE and set it up for use with Emacs.
>>
>> I wonder if we even have to mention the "use with Emacs"?  What else
>> could it mean?  The only issue I can think of is if someone hasn't yet
>> understood that package.el & co. are elisp package managers.
>>
>
> I don't have a strong preference about this one either way.

As long as the docstring remains short enough, it should be fine.

>>> +
>>> +PACKAGE specifies which package to install, where to find its
>>> +source repository and how to build it.
>>> +
>>> +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.
>>
>> AND the package archive hosts elpa-packages.eld, UNLESS a heuristic can
>> be used to guess the repository.
>>
>
> Do you think we need to include these details in the docstring?  It
> feels a bit too nitty gritty so I wonder if we can avoid it.  Are there
> common use case in which this extra information is significant?

The thing is, it should be explained somewhere (and I might have just
forgotten that i was explained at some point).  Perhaps the manual is a
better place to go into these complications.

The patch looks fine to me, but considering my history with
documentation I think it is best to let a few others take a look at your
revision as well.  Other than that, the plan is to pull out the
`package-vc-default-backend' custom type and rename
`package-vc-allow-side-effects', right?