bug#66567: [PATCH] use-package: Add ignored-files support to :vc keyword

[Philip Kaludercic]

Tony Zorman <tonyzorman@mailbox.org> writes:

> Hi,
>
> this patch augments use-package's :vc keyword with the ability to ignore
> files. This is according to the functionality added to package-vc.el in
> 68318dfd16. There is also another small commit lurking in there that
> enables support for :make and :shell-command, both of which were added
> in 5ac08768aa.
>
>   Tony
>
>>From 2b3c81c1854dc4767105021a6a777c6cb1b04bca Mon Sep 17 00:00:00 2001
> From: Tony Zorman <soliditsallgood@mailbox.org>
> Date: Sun, 15 Oct 2023 16:50:00 +0200
> Subject: [PATCH] ; use-package: Update list of valid :vc keywords
>
> lisp/use-package/use-package-core.el: Add :shell-command, :make to
> valid keywords.
> ---
>  lisp/use-package/use-package-core.el | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/lisp/use-package/use-package-core.el 
> b/lisp/use-package/use-package-core.el
> index 34c45b7aec..5d0d554baf 100644
> --- a/lisp/use-package/use-package-core.el
> +++ b/lisp/use-package/use-package-core.el
> @@ -1654,7 +1654,8 @@ use-package-normalize--vc-arg
>                               (t (ensure-string v))))
>                   (:vc-backend (ensure-symbol v))
>                   (_ (ensure-string v)))))
> -    (pcase-let ((valid-kws '(:url :branch :lisp-dir :main-file :vc-backend 
> :rev))
> +    (pcase-let ((valid-kws '( :url :branch :lisp-dir :main-file :vc-backend 
> :rev
> +                              :shell-command :make))

Why is use-package checking for valid keywords in the first place?

>                  (`(,name . ,opts) arg))
>        (if (stringp opts)                ; (NAME . VERSION-STRING) ?
>            (list name opts)
> -- 
> 2.42.0
>
>>From baf9490a3f3b18a336fe8c860f820beda01ba131 Mon Sep 17 00:00:00 2001
> From: Tony Zorman <soliditsallgood@mailbox.org>
> Date: Sun, 15 Oct 2023 16:51:00 +0200
> Subject: [PATCH] use-package: Add :ignored-files support to :vc keyword
>
> * lisp/use-package/use-package-core.el (use-package-split-when):
> New utility function to split a list whenever a specified predicate
> returns t.
> (use-package-vc-valid-keywords): A new defconst to gather all allowed
> keywords.
> (use-package-normalize--vc-arg): Properly normalize the :ignored-files
> keyword, in that the following are all valid ways of entering files:
>   :ignored-files "a"
>   :ignored-files ("a")
>   :ignored-files "a" "b" "c"
>   :ignored-files ("a" "b" "c")
> (use-package-normalize/:vc): Adjust normalization, now that we do not
> necessarily receive a valid plist as an input.

I would much prefer that package specifications have a canonical form
and that use-package doesn't try to introduce variations that wouldn't
be compatible with package-vc-install proper and elpa-admin.  Or is this
necessary for use-package?

>
> * test/lisp/use-package/use-package-tests.el (use-package-test-normalize/:vc):
> Add tests for :ignored-files keyword.
> ---
>  lisp/use-package/use-package-core.el       | 61 ++++++++++++++++------
>  test/lisp/use-package/use-package-tests.el | 10 +++-
>  2 files changed, 53 insertions(+), 18 deletions(-)
>
> diff --git a/lisp/use-package/use-package-core.el 
> b/lisp/use-package/use-package-core.el
> index 5d0d554baf..1c2e4676d7 100644
> --- a/lisp/use-package/use-package-core.el
> +++ b/lisp/use-package/use-package-core.el
> @@ -521,6 +521,24 @@ use-package-split-list-at-keys
>         (let ((xs (use-package-split-list (apply-partially #'eq key) lst)))
>           (cons (car xs) (use-package-split-list-at-keys key (cddr xs))))))
>  
> +(defun use-package-split-when (pred xs)
> +  "Repeatedly split a list according to PRED.
> +Split XS every time PRED returns t.  Keep the delimiters, and
> +arrange the result in an alist.  For example:
> +
> +  (use-package-split-when #\\='keywordp \\='(:a 1 :b 2 3 4 :c 5))
> +  ;; => \\='((:a 1) (:b 2 3 4) (:c 5))
> +
> +  (use-package-split-when (lambda (x) (> x 2)) \\='(10 1 3 2 4 -1 8 9))
> +  ;; => \\='((10 1) (3 2) (4 -1) (8) (9))"
> +  (unless (seq-empty-p xs)
> +    (pcase-let* ((`(,first . ,rest) (if (funcall pred (car xs))
> +                                        (cons (car xs) (cdr xs))
> +                                      (use-package-split-list pred xs)))
> +                 (`(,val . ,recur) (use-package-split-list pred rest)))
> +      (cons (cons first val)
> +            (use-package-split-when pred recur)))))
> +
>  
> ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
>  ;;
>  ;;; Keywords
> @@ -1634,6 +1652,12 @@ use-package-handler/:vc
>        (push `(use-package-vc-install ',arg ,local-path) body))   ; runtime
>      body))
>  
> +(defconst use-package-vc-valid-keywords
> +  '( :url :branch :lisp-dir :main-file :vc-backend :rev
> +     :shell-command :make :ignored-files)
> +  "Valid keywords for the `:vc' keyword, see the Info
> +node `(emacs)Fetching Package Sources'.")
> +
>  (defun use-package-normalize--vc-arg (arg)
>    "Normalize possible arguments to the `:vc' keyword.
>  ARG is a cons-cell of approximately the form that
> @@ -1653,24 +1677,27 @@ use-package-normalize--vc-arg
>                               ((eq v :newest) nil)
>                               (t (ensure-string v))))
>                   (:vc-backend (ensure-symbol v))
> +                 (:ignored-files (if (listp v) v (list v)))
>                   (_ (ensure-string v)))))
> -    (pcase-let ((valid-kws '( :url :branch :lisp-dir :main-file :vc-backend 
> :rev
> -                              :shell-command :make))
> -                (`(,name . ,opts) arg))
> +    (pcase-let* ((`(,name . ,opts) arg))
>        (if (stringp opts)                ; (NAME . VERSION-STRING) ?
>            (list name opts)
> -        ;; Error handling
> -        (cl-loop for (k _) on opts by #'cddr
> -                 if (not (member k valid-kws))
> -                 do (use-package-error
> -                     (format "Keyword :vc received unknown argument: %s. 
> Supported keywords are: %s"
> -                             k valid-kws)))
> -        ;; Actual normalization
> -        (list name
> -              (cl-loop for (k v) on opts by #'cddr
> -                       if (not (eq k :rev))
> -                       nconc (list k (normalize k v)))
> -              (normalize :rev (plist-get opts :rev)))))))
> +        (let ((opts (use-package-split-when
> +                     (lambda (el)
> +                       (seq-contains-p use-package-vc-valid-keywords el))
> +                     opts)))
> +          ;; Error handling
> +          (cl-loop for (k . _) in opts
> +                   if (not (member k use-package-vc-valid-keywords))
> +                   do (use-package-error
> +                       (format "Keyword :vc received unknown argument: %s. 
> Supported keywords are: %s"
> +                               k use-package-vc-valid-keywords)))
> +          ;; Actual normalization
> +          (list name
> +                (cl-loop for (k . v) in opts
> +                         if (not (eq k :rev))
> +                         nconc (list k (normalize k (if (length= v 1) (car 
> v) v))))
> +                (normalize :rev (car (alist-get :rev opts)))))))))
>  
>  (defun use-package-normalize/:vc (name _keyword args)
>    "Normalize possible arguments to the `:vc' keyword.
> @@ -1686,9 +1713,9 @@ use-package-normalize/:vc
>        ((or 'nil 't) (list name))                 ; guess name
>        ((pred symbolp) (list arg))                ; use this name
>        ((pred stringp) (list name arg))           ; version string + guess 
> name
> -      ((pred plistp)                             ; plist + guess name
> +      (`(,(pred keywordp) . ,(pred listp))       ; list + guess name
>         (use-package-normalize--vc-arg (cons name arg)))
> -      (`(,(pred symbolp) . ,(or (pred plistp)    ; plist/version string + 
> name
> +      (`(,(pred symbolp) . ,(or (pred listp)     ; list/version string + name
>                                  (pred stringp)))
>         (use-package-normalize--vc-arg arg))
>        (_ (use-package-error "Unrecognised argument to :vc.\
> diff --git a/test/lisp/use-package/use-package-tests.el 
> b/test/lisp/use-package/use-package-tests.el
> index 9181a8171a..5636ba8a4f 100644
> --- a/test/lisp/use-package/use-package-tests.el
> +++ b/test/lisp/use-package/use-package-tests.el
> @@ -2014,7 +2014,15 @@ use-package-test-normalize/:vc
>    (should (equal '(foo)
>                   (use-package-normalize/:vc 'foo :vc nil)))
>    (should (equal '(bar)
> -                 (use-package-normalize/:vc 'foo :vc '(bar)))))
> +                 (use-package-normalize/:vc 'foo :vc '(bar))))
> +  (should (equal
> +           '(foo (:ignored-files ("a" "b" "c")) :last-release)
> +           (use-package-normalize/:vc 'foo :vc '((:ignored-files "a" "b" 
> "c")))))
> +  (should (equal
> +           (use-package-normalize/:vc 'foo :vc '((:ignored-files "a")))
> +           (use-package-normalize/:vc 'foo :vc '((:ignored-files ("a"))))))
> +  (should (equal (use-package-normalize/:vc 'foo :vc '((:ignored-files "a" 
> "b" "c")))
> +                 (use-package-normalize/:vc 'foo :vc '((:ignored-files ("a" 
> "b" "c")))))))
>  
>  ;; Local Variables:
>  ;; no-byte-compile: t
> -- 
> 2.42.0

Tony Zorman <tonyzorman@mailbox.org> writes:

> I will cheekily bump this, and also Cc. Philip as the most likely
> reviewer.

I don't use use-package nor am I familiar with the code base, so I
wouldn't value my input that much.

>   Tony

-- 
Philip Kaludercic