From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.io!.POSTED.blaine.gmane.org!not-for-mail From: Eli Zaretskii Newsgroups: gmane.emacs.bugs Subject: bug#65386: [PATCH] ; Refine some 'package-vc' docstrings Date: Sat, 19 Aug 2023 22:47:12 +0300 Message-ID: <83msymyfun.fsf@gnu.org> References: Injection-Info: ciao.gmane.io; posting-host="blaine.gmane.org:116.202.254.214"; logging-data="34479"; mail-complaints-to="usenet@ciao.gmane.io" Cc: philipk@posteo.net, 65386@debbugs.gnu.org To: Eshel Yaron Original-X-From: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane-mx.org@gnu.org Sat Aug 19 21:48:25 2023 Return-path: Envelope-to: geb-bug-gnu-emacs@m.gmane-mx.org Original-Received: from lists.gnu.org ([209.51.188.17]) by ciao.gmane.io with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1qXRvs-0008m7-A1 for geb-bug-gnu-emacs@m.gmane-mx.org; Sat, 19 Aug 2023 21:48:24 +0200 Original-Received: from localhost ([::1] helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1qXRvX-0005hs-Ev; Sat, 19 Aug 2023 15:48:03 -0400 Original-Received: from eggs.gnu.org ([2001:470:142:3::10]) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1qXRvV-0005hd-NR for bug-gnu-emacs@gnu.org; Sat, 19 Aug 2023 15:48:01 -0400 Original-Received: from debbugs.gnu.org ([2001:470:142:5::43]) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.90_1) (envelope-from ) id 1qXRvV-00082e-FU for bug-gnu-emacs@gnu.org; Sat, 19 Aug 2023 15:48:01 -0400 Original-Received: from Debian-debbugs by debbugs.gnu.org with local (Exim 4.84_2) (envelope-from ) id 1qXRvW-0000YS-70 for bug-gnu-emacs@gnu.org; Sat, 19 Aug 2023 15:48:02 -0400 X-Loop: help-debbugs@gnu.org Resent-From: Eli Zaretskii Original-Sender: "Debbugs-submit" Resent-CC: bug-gnu-emacs@gnu.org Resent-Date: Sat, 19 Aug 2023 19:48:02 +0000 Resent-Message-ID: Resent-Sender: help-debbugs@gnu.org X-GNU-PR-Message: followup 65386 X-GNU-PR-Package: emacs X-GNU-PR-Keywords: patch Original-Received: via spool by 65386-submit@debbugs.gnu.org id=B65386.16924744331901 (code B ref 65386); Sat, 19 Aug 2023 19:48:02 +0000 Original-Received: (at 65386) by debbugs.gnu.org; 19 Aug 2023 19:47:13 +0000 Original-Received: from localhost ([127.0.0.1]:52326 helo=debbugs.gnu.org) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1qXRuh-0000UZ-T5 for submit@debbugs.gnu.org; Sat, 19 Aug 2023 15:47:13 -0400 Original-Received: from eggs.gnu.org ([2001:470:142:3::10]:45026) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1qXRud-0000UJ-N7 for 65386@debbugs.gnu.org; Sat, 19 Aug 2023 15:47:11 -0400 Original-Received: from fencepost.gnu.org ([2001:470:142:3::e]) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1qXRuW-0007mP-1f; Sat, 19 Aug 2023 15:47:00 -0400 DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=gnu.org; s=fencepost-gnu-org; h=References:Subject:In-Reply-To:To:From:Date: mime-version; bh=g4ulsh3AUqgjxKrd2VJxGUIk/4Z/3wKZQOZ5QyOL24A=; b=iBbebxCeaoSA VjowG0P88jSKvolNQtwhlwNnhzxsMGuK534e3dZK7TvgCQL6n3ljn8NROmsFcjhBSgDvm0xVeDJxu 28ZIhnibvUeSoO9m52HMqxVQnDZbJwzx1MHuDf3JBmGTOVMMJ/D7VKnTABZ8IycXrGVgODJPtwc1+ WBYN6LicKni11v/GrwuhA6UiDz2cRbegP3H6NS4mm36bpM0QXK5xF09O4PGNz1PCfA7HD7CkGVUEo hTIBrqygu/FB9f732vZt8kLlFJ0eOf1h+5QChJeBl4Onl8YM4rgsb0jgYSQmfaN9PXaWUhIWcYdg5 lirWvlLQbJy7rs0iIlueOg==; In-Reply-To: (bug-gnu-emacs@gnu.org) X-BeenThere: debbugs-submit@debbugs.gnu.org X-Mailman-Version: 2.1.18 Precedence: list X-BeenThere: bug-gnu-emacs@gnu.org List-Id: "Bug reports for GNU Emacs, the Swiss army knife of text editors" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane-mx.org@gnu.org Original-Sender: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane-mx.org@gnu.org Xref: news.gmane.io gmane.emacs.bugs:267910 Archived-At: > 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: > @@ -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. > +`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. > (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'. > + > +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? > +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. > (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? > + 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." > (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. > +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. > (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." > (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.) > -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. > +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. > (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".