From: Philip Kaludercic <philipk@posteo.net>
To: Eshel Yaron <me@eshelyaron.com>
Cc: Eli Zaretskii <eliz@gnu.org>, 65386@debbugs.gnu.org
Subject: bug#65386: [PATCH] ; Refine some 'package-vc' docstrings
Date: Sun, 20 Aug 2023 12:18:59 +0000 [thread overview]
Message-ID: <87ttstucss.fsf@posteo.net> (raw)
In-Reply-To: <m1il9a81gn.fsf@eshelyaron.com> (Eshel Yaron's message of "Sun, 20 Aug 2023 12:14:48 +0200")
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?
next prev parent reply other threads:[~2023-08-20 12:18 UTC|newest]
Thread overview: 22+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-08-19 18:07 bug#65386: [PATCH] ; Refine some 'package-vc' docstrings Eshel Yaron via Bug reports for GNU Emacs, the Swiss army knife of text editors
2023-08-19 19:47 ` Eli Zaretskii
2023-08-19 20:42 ` Eshel Yaron via Bug reports for GNU Emacs, the Swiss army knife of text editors
2023-08-20 6:05 ` Eli Zaretskii
2023-08-20 7:15 ` Eshel Yaron via Bug reports for GNU Emacs, the Swiss army knife of text editors
2023-08-20 8:32 ` Philip Kaludercic
2023-08-20 10:14 ` Eshel Yaron via Bug reports for GNU Emacs, the Swiss army knife of text editors
2023-08-20 12:18 ` Philip Kaludercic [this message]
2023-08-20 14:44 ` Eshel Yaron via Bug reports for GNU Emacs, the Swiss army knife of text editors
2023-08-20 16:04 ` Philip Kaludercic
2023-08-20 16:22 ` Eshel Yaron via Bug reports for GNU Emacs, the Swiss army knife of text editors
2023-08-20 18:33 ` Philip Kaludercic
2023-08-20 10:31 ` Eshel Yaron via Bug reports for GNU Emacs, the Swiss army knife of text editors
2023-08-20 8:13 ` Philip Kaludercic
2023-08-20 19:24 ` Mauro Aranda
2023-08-20 20:35 ` Philip Kaludercic
2023-08-21 8:16 ` Eshel Yaron via Bug reports for GNU Emacs, the Swiss army knife of text editors
2023-08-21 9:27 ` Philip Kaludercic
2023-08-21 10:22 ` Eshel Yaron via Bug reports for GNU Emacs, the Swiss army knife of text editors
[not found] <87ledwipkb.fsf@posteo.net>
2023-08-27 17:18 ` Eshel Yaron via Bug reports for GNU Emacs, the Swiss army knife of text editors
2023-08-29 8:43 ` Philip Kaludercic
2023-08-29 9:10 ` Eshel Yaron via Bug reports for GNU Emacs, the Swiss army knife of text editors
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: https://www.gnu.org/software/emacs/
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=87ttstucss.fsf@posteo.net \
--to=philipk@posteo.net \
--cc=65386@debbugs.gnu.org \
--cc=eliz@gnu.org \
--cc=me@eshelyaron.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://git.savannah.gnu.org/cgit/emacs.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).