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

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 66567 in the body.
You can then email your comments to 66567 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Tony Zorman <tonyzorman <at> mailbox.org>
To: bug-gnu-emacs <at> gnu.org
Subject: [PATCH] use-package: Add ignored-files support to :vc keyword 
Date: Sun, 15 Oct 2023 18:42:16 +0200

[Message part 1 (text/plain, inline)]

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

[0001-use-package-Update-list-of-valid-vc-keywords.patch (text/x-patch, inline)]

From 2b3c81c1854dc4767105021a6a777c6cb1b04bca Mon Sep 17 00:00:00 2001
From: Tony Zorman <soliditsallgood <at> 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))
                 (`(,name . ,opts) arg))
       (if (stringp opts)                ; (NAME . VERSION-STRING) ?
           (list name opts)
-- 
2.42.0

[0002-use-package-Add-ignored-files-support-to-vc-keyword.patch (text/x-patch, inline)]

From baf9490a3f3b18a336fe8c860f820beda01ba131 Mon Sep 17 00:00:00 2001
From: Tony Zorman <soliditsallgood <at> 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.

* 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

[Message part 4 (text/plain, inline)]

-- 
Tony Zorman | https://tony-zorman.com/

Message #10 received at 66567 <at> debbugs.gnu.org (full text, mbox):

From: Tony Zorman <tonyzorman <at> mailbox.org>
To: 66567 <at> debbugs.gnu.org
Cc: Philip Kaludercic <philipk <at> posteo.net>
Subject: Re: [PATCH] use-package: Add ignored-files support to :vc keyword 
Date: Wed, 01 Nov 2023 08:43:23 +0100

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

  Tony

-- 
Tony Zorman | https://tony-zorman.com/

Message #13 received at 66567 <at> debbugs.gnu.org (full text, mbox):

From: Philip Kaludercic <philipk <at> posteo.net>
To: Tony Zorman <tonyzorman <at> mailbox.org>
Cc: 66567 <at> debbugs.gnu.org
Subject: Re: bug#66567: [PATCH] use-package: Add ignored-files support to
 :vc keyword
Date: Wed, 01 Nov 2023 09:09:20 +0000

Tony Zorman <tonyzorman <at> 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 <at> 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 <at> 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 <at> 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

Message #16 received at 66567 <at> debbugs.gnu.org (full text, mbox):

From: Tony Zorman <tonyzorman <at> mailbox.org>
To: Philip Kaludercic <philipk <at> posteo.net>
Cc: 66567 <at> debbugs.gnu.org
Subject: Re: bug#66567: [PATCH] use-package: Add ignored-files support to
 :vc keyword
Date: Wed, 01 Nov 2023 11:13:25 +0100

On Wed, Nov 01 2023 09:09, Philip Kaludercic wrote:
>> 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?

Better error messages, mostly. Especially people switching from
quelpa/straight/vc-use-package might be surprised that :vc is not a
drop-in replacement for those packages. I feel like alerting them to
this fact sooner rather than later makes for a better experience.

>> * 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?

It's not *necessary*, but it's quite common for use-package keywords to
do their best in order to be as unobtrusive as possible. This includes
omitting parentheses that might not be strictly needed, or to cleverly
transform the input in some other way (e.g., :hook makes great use of
this).

>> 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.

Oh, fair enough. In either case, I couldn't think of anyone else—sorry
for the noise :)

-- 
Tony Zorman | https://tony-zorman.com/

Message #19 received at 66567 <at> debbugs.gnu.org (full text, mbox):

From: Philip Kaludercic <philipk <at> posteo.net>
To: Tony Zorman <tonyzorman <at> mailbox.org>
Cc: 66567 <at> debbugs.gnu.org
Subject: Re: bug#66567: [PATCH] use-package: Add ignored-files support to
 :vc keyword
Date: Wed, 01 Nov 2023 12:48:47 +0000

Tony Zorman <tonyzorman <at> mailbox.org> writes:

> On Wed, Nov 01 2023 09:09, Philip Kaludercic wrote:
>>> 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?
>
> Better error messages, mostly. Especially people switching from
> quelpa/straight/vc-use-package might be surprised that :vc is not a
> drop-in replacement for those packages. I feel like alerting them to
> this fact sooner rather than later makes for a better experience.

IIUC this would raise an error when an unknown keyword is encountered,
right?

>>> * 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?
>
> It's not *necessary*, but it's quite common for use-package keywords to
> do their best in order to be as unobtrusive as possible. This includes
> omitting parentheses that might not be strictly needed, or to cleverly
> transform the input in some other way (e.g., :hook makes great use of
> this).

My experience is that this is more likely to cause confusion, e.g. when
people write :config (progn ...).  But as I said, since I don't use
use-package, I don't want to give the final verdict, I am just
expressing my preferences.

>>> 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.
>
> Oh, fair enough. In either case, I couldn't think of anyone else—sorry
> for the noise :)

I think that Stefan Kangas would probably be the best to ask, since he
was the one responsible for merging use-package into the core.

Message #22 received at 66567 <at> debbugs.gnu.org (full text, mbox):

From: Tony Zorman <tonyzorman <at> mailbox.org>
To: Philip Kaludercic <philipk <at> posteo.net>
Cc: stefankangas <at> gmail.com, 66567 <at> debbugs.gnu.org
Subject: Re: bug#66567: [PATCH] use-package: Add ignored-files support to
 :vc keyword
Date: Wed, 01 Nov 2023 15:36:58 +0100

[Message part 1 (text/plain, inline)]

On Wed, Nov 01 2023 12:48, Philip Kaludercic wrote:
> Tony Zorman <tonyzorman <at> mailbox.org> writes:
>
>> On Wed, Nov 01 2023 09:09, Philip Kaludercic wrote:
>>>> 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?
>>
>> Better error messages, mostly. Especially people switching from
>> quelpa/straight/vc-use-package might be surprised that :vc is not a
>> drop-in replacement for those packages. I feel like alerting them to
>> this fact sooner rather than later makes for a better experience.
>
> IIUC this would raise an error when an unknown keyword is encountered,
> right?

Yes, a declaration like

    (use-package foo
      :vc (:url "url" :blargh "123"))

would result in the following message

    ⛔ Error (use-package): Failed to parse package foo: use-package: Keyword :vc received unknown argument: :blargh. Supported keywords are: (:url :branch :lisp-dir :main-file :vc-backend :rev :shell-command :make :ignored-files)

Things get a bit muddier if ':blargh' would be passed down to
package-vc-install.

Now that you mention it, I noticed a tiny mistake (resulting in a worse
error message!) in the second of the original patches. I've attached a
corrected version.

>>>> 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.
>>
>> Oh, fair enough. In either case, I couldn't think of anyone else—sorry
>> for the noise :)
>
> I think that Stefan Kangas would probably be the best to ask, since he
> was the one responsible for merging use-package into the core.

Thanks! I have Cc'd Stefan, hoping to not come across as too pushy :)

  Tony

[0002-use-package-Add-ignored-files-support-to-vc-keyword.patch (text/x-patch, inline)]

From f8590d37b29a96a7984d8acae2d6c2b557e0dc59 Mon Sep 17 00:00:00 2001
From: Tony Zorman <soliditsallgood <at> 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.

* 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       | 60 ++++++++++++++++------
 test/lisp/use-package/use-package-tests.el | 10 +++-
 2 files changed, 52 insertions(+), 18 deletions(-)

diff --git a/lisp/use-package/use-package-core.el b/lisp/use-package/use-package-core.el
index 5d0d554baf..974059a850 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,26 @@ 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) (and (keywordp el) (not (equal :newest 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 +1712,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

[Message part 3 (text/plain, inline)]

-- 
Tony Zorman | https://tony-zorman.com/

Message #25 received at 66567 <at> debbugs.gnu.org (full text, mbox):

From: Philip Kaludercic <philipk <at> posteo.net>
To: Tony Zorman <tonyzorman <at> mailbox.org>
Cc: stefankangas <at> gmail.com, 66567 <at> debbugs.gnu.org
Subject: Re: bug#66567: [PATCH] use-package: Add ignored-files support to
 :vc keyword
Date: Wed, 01 Nov 2023 16:38:53 +0000

Tony Zorman <tonyzorman <at> mailbox.org> writes:

> On Wed, Nov 01 2023 12:48, Philip Kaludercic wrote:
>> Tony Zorman <tonyzorman <at> mailbox.org> writes:
>>
>>> On Wed, Nov 01 2023 09:09, Philip Kaludercic wrote:
>>>>> 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?
>>>
>>> Better error messages, mostly. Especially people switching from
>>> quelpa/straight/vc-use-package might be surprised that :vc is not a
>>> drop-in replacement for those packages. I feel like alerting them to
>>> this fact sooner rather than later makes for a better experience.
>>
>> IIUC this would raise an error when an unknown keyword is encountered,
>> right?
>
> Yes, a declaration like
>
>     (use-package foo
>       :vc (:url "url" :blargh "123"))
>
> would result in the following message
>
>     ⛔ Error (use-package): Failed to parse package foo: use-package: Keyword :vc received unknown argument: :blargh. Supported keywords are: (:url :branch :lisp-dir :main-file :vc-backend :rev :shell-command :make :ignored-files)
>
> Things get a bit muddier if ':blargh' would be passed down to
> package-vc-install.

What I was wondering, was if it would make sense to raise an warning
instead.

> now that you mention it, I noticed a tiny mistake (resulting in a worse
> error message!) in the second of the original patches. I've attached a
> corrected version.

1+

>>>>> 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.
>>>
>>> Oh, fair enough. In either case, I couldn't think of anyone else—sorry
>>> for the noise :)
>>
>> I think that Stefan Kangas would probably be the best to ask, since he
>> was the one responsible for merging use-package into the core.
>
> Thanks! I have Cc'd Stefan, hoping to not come across as too pushy :)

That should be fine (I hope).

>   Tony
>
> From f8590d37b29a96a7984d8acae2d6c2b557e0dc59 Mon Sep 17 00:00:00 2001
> From: Tony Zorman <soliditsallgood <at> 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.
>
> * 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       | 60 ++++++++++++++++------
>  test/lisp/use-package/use-package-tests.el | 10 +++-
>  2 files changed, 52 insertions(+), 18 deletions(-)
>
> diff --git a/lisp/use-package/use-package-core.el b/lisp/use-package/use-package-core.el
> index 5d0d554baf..974059a850 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,26 @@ 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) (and (keywordp el) (not (equal :newest 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 +1712,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

Message #28 received at 66567 <at> debbugs.gnu.org (full text, mbox):

From: Tony Zorman <tonyzorman <at> mailbox.org>
To: Philip Kaludercic <philipk <at> posteo.net>
Cc: stefankangas <at> gmail.com, 66567 <at> debbugs.gnu.org
Subject: Re: bug#66567: [PATCH] use-package: Add ignored-files support to
 :vc keyword
Date: Tue, 07 Nov 2023 20:39:07 +0100

On Wed, Nov 01 2023 16:38, Philip Kaludercic wrote:
> Tony Zorman <tonyzorman <at> mailbox.org> writes:
>> On Wed, Nov 01 2023 12:48, Philip Kaludercic wrote:
>>> Tony Zorman <tonyzorman <at> mailbox.org> writes:
>>>> On Wed, Nov 01 2023 09:09, Philip Kaludercic wrote:
>>>>
>>>>> Why is use-package checking for valid keywords in the first place?
>>>>
>>>> Better error messages, mostly. Especially people switching from
>>>> quelpa/straight/vc-use-package might be surprised that :vc is not a
>>>> drop-in replacement for those packages. I feel like alerting them to
>>>> this fact sooner rather than later makes for a better experience.
>>>
>>> IIUC this would raise an error when an unknown keyword is encountered,
>>> right?
>>
>> Yes, a declaration like
>>
>>     (use-package foo
>>       :vc (:url "url" :blargh "123"))
>>
>> would result in the following message
>>
>>     ⛔ Error (use-package): Failed to parse package foo: use-package: Keyword :vc received unknown argument: :blargh. Supported keywords are: (:url :branch :lisp-dir :main-file :vc-backend :rev :shell-command :make :ignored-files)
>>
>> Things get a bit muddier if ':blargh' would be passed down to
>> package-vc-install.
>
> What I was wondering, was if it would make sense to raise an warning
> instead.

Now I'm a bit confused: where exactly? Inside of use-package or
package-vc? Either way, I think raising an error when the user inputs
nonsense is totally justified—I'd just like that error to be
understandable as quickly as possible.

  Tony

-- 
Tony Zorman | https://tony-zorman.com/

Message #31 received at 66567 <at> debbugs.gnu.org (full text, mbox):

From: Philip Kaludercic <philipk <at> posteo.net>
To: Tony Zorman <tonyzorman <at> mailbox.org>
Cc: stefankangas <at> gmail.com, 66567 <at> debbugs.gnu.org
Subject: Re: bug#66567: [PATCH] use-package: Add ignored-files support to
 :vc keyword
Date: Tue, 07 Nov 2023 21:24:58 +0000

Tony Zorman <tonyzorman <at> mailbox.org> writes:

> On Wed, Nov 01 2023 16:38, Philip Kaludercic wrote:
>> Tony Zorman <tonyzorman <at> mailbox.org> writes:
>>> On Wed, Nov 01 2023 12:48, Philip Kaludercic wrote:
>>>> Tony Zorman <tonyzorman <at> mailbox.org> writes:
>>>>> On Wed, Nov 01 2023 09:09, Philip Kaludercic wrote:
>>>>>
>>>>>> Why is use-package checking for valid keywords in the first place?
>>>>>
>>>>> Better error messages, mostly. Especially people switching from
>>>>> quelpa/straight/vc-use-package might be surprised that :vc is not a
>>>>> drop-in replacement for those packages. I feel like alerting them to
>>>>> this fact sooner rather than later makes for a better experience.
>>>>
>>>> IIUC this would raise an error when an unknown keyword is encountered,
>>>> right?
>>>
>>> Yes, a declaration like
>>>
>>>     (use-package foo
>>>       :vc (:url "url" :blargh "123"))
>>>
>>> would result in the following message
>>>
>>>     ⛔ Error (use-package): Failed to parse package foo: use-package: Keyword :vc received unknown argument: :blargh. Supported keywords are: (:url :branch :lisp-dir :main-file :vc-backend :rev :shell-command :make :ignored-files)
>>>
>>> Things get a bit muddier if ':blargh' would be passed down to
>>> package-vc-install.
>>
>> What I was wondering, was if it would make sense to raise an warning
>> instead.
>
> Now I'm a bit confused: where exactly? Inside of use-package or
> package-vc? Either way, I think raising an error when the user inputs
> nonsense is totally justified—I'd just like that error to be
> understandable as quickly as possible.

I was thinking that package-vc should emit an error, but that
use-package could emit a warning, in case a new keyword is added to
package-vc specifications but hasn't yet been added to the use-package
layer -- mainly because I don't use the latter and am not that familiar
with the code.

>   Tony

Message #34 received at 66567 <at> debbugs.gnu.org (full text, mbox):

From: Tony Zorman <tonyzorman <at> mailbox.org>
To: Philip Kaludercic <philipk <at> posteo.net>
Cc: stefankangas <at> gmail.com, 66567 <at> debbugs.gnu.org
Subject: Re: bug#66567: [PATCH] use-package: Add ignored-files support to
 :vc keyword
Date: Fri, 10 Nov 2023 13:00:12 +0100

On Tue, Nov 07 2023 21:24, Philip Kaludercic wrote:
> Tony Zorman <tonyzorman <at> mailbox.org> writes:
>> Now I'm a bit confused: where exactly? Inside of use-package or
>> package-vc? Either way, I think raising an error when the user inputs
>> nonsense is totally justified—I'd just like that error to be
>> understandable as quickly as possible.
>
> I was thinking that package-vc should emit an error, but that
> use-package could emit a warning, in case a new keyword is added to
> package-vc specifications but hasn't yet been added to the use-package
> layer -- mainly because I don't use the latter and am not that familiar
> with the code.

On first thought this sounds good, but I think one would end up having
to sync the actual state of things in two places. What if package-vc.el
had a place where it stored all available commands? Then the use-package
integration could just use that. I think it would also be easier to have
this codified somewhere, rather than having to check the manual every
time.

-- 
Tony Zorman | https://tony-zorman.com/

Message #37 received at 66567 <at> debbugs.gnu.org (full text, mbox):

From: Philip Kaludercic <philipk <at> posteo.net>
To: Tony Zorman <tonyzorman <at> mailbox.org>
Cc: stefankangas <at> gmail.com, 66567 <at> debbugs.gnu.org
Subject: Re: bug#66567: [PATCH] use-package: Add ignored-files support to
 :vc keyword
Date: Thu, 16 Nov 2023 07:32:17 +0000

Tony Zorman <tonyzorman <at> mailbox.org> writes:

> On Tue, Nov 07 2023 21:24, Philip Kaludercic wrote:
>> Tony Zorman <tonyzorman <at> mailbox.org> writes:
>>> Now I'm a bit confused: where exactly? Inside of use-package or
>>> package-vc? Either way, I think raising an error when the user inputs
>>> nonsense is totally justified—I'd just like that error to be
>>> understandable as quickly as possible.
>>
>> I was thinking that package-vc should emit an error, but that
>> use-package could emit a warning, in case a new keyword is added to
>> package-vc specifications but hasn't yet been added to the use-package
>> layer -- mainly because I don't use the latter and am not that familiar
>> with the code.
>
> On first thought this sounds good, but I think one would end up having
> to sync the actual state of things in two places. What if package-vc.el
> had a place where it stored all available commands? Then the use-package
> integration could just use that. I think it would also be easier to have
> this codified somewhere, rather than having to check the manual every
> time.

This sounds like a better solution, I can add a constant with all the
symbols that you can then use.  I am still a bit busy, so I cannot
promise when I'll manage to do so, but I'll ping you when the changes
have been pushed.

Message #40 received at 66567 <at> debbugs.gnu.org (full text, mbox):

From: Tony Zorman <tonyzorman <at> mailbox.org>
To: Philip Kaludercic <philipk <at> posteo.net>
Cc: 66567 <at> debbugs.gnu.org
Subject: Re: bug#66567: [PATCH] use-package: Add ignored-files support to
 :vc keyword
Date: Thu, 02 May 2024 20:57:41 +0200

On Thu, Nov 16 2023 07:32, Philip Kaludercic wrote:
> Tony Zorman <tonyzorman <at> mailbox.org> writes:
>
>> On Tue, Nov 07 2023 21:24, Philip Kaludercic wrote:
>>> Tony Zorman <tonyzorman <at> mailbox.org> writes:
>>>> Now I'm a bit confused: where exactly? Inside of use-package or
>>>> package-vc? Either way, I think raising an error when the user inputs
>>>> nonsense is totally justified—I'd just like that error to be
>>>> understandable as quickly as possible.
>>>
>>> I was thinking that package-vc should emit an error, but that
>>> use-package could emit a warning, in case a new keyword is added to
>>> package-vc specifications but hasn't yet been added to the use-package
>>> layer -- mainly because I don't use the latter and am not that familiar
>>> with the code.
>>
>> On first thought this sounds good, but I think one would end up having
>> to sync the actual state of things in two places. What if package-vc.el
>> had a place where it stored all available commands? Then the use-package
>> integration could just use that. I think it would also be easier to have
>> this codified somewhere, rather than having to check the manual every
>> time.
>
> This sounds like a better solution, I can add a constant with all the
> symbols that you can then use.  I am still a bit busy, so I cannot
> promise when I'll manage to do so, but I'll ping you when the changes
> have been pushed.

Can I perhaps bump this myself instead? :)

I'd really like to see this merged; if it helps move things along a
bit quicker, I can also make the necessary changes to package-vc.el.

  Tony

-- 
Tony Zorman | https://tony-zorman.com/

Message #43 received at 66567 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Tony Zorman <tonyzorman <at> mailbox.org>,
 John Wiegley <johnw <at> gnu.org>
Cc: philipk <at> posteo.net, 66567 <at> debbugs.gnu.org
Subject: Re: bug#66567: [PATCH] use-package: Add ignored-files support to :vc
 keyword
Date: Fri, 03 May 2024 09:35:19 +0300

> Cc: 66567 <at> debbugs.gnu.org
> Date: Thu, 02 May 2024 20:57:41 +0200
> From:  Tony Zorman via "Bug reports for GNU Emacs,
>  the Swiss army knife of text editors" <bug-gnu-emacs <at> gnu.org>
> 
> On Thu, Nov 16 2023 07:32, Philip Kaludercic wrote:
> > Tony Zorman <tonyzorman <at> mailbox.org> writes:
> >
> >> On Tue, Nov 07 2023 21:24, Philip Kaludercic wrote:
> >>> Tony Zorman <tonyzorman <at> mailbox.org> writes:
> >>>> Now I'm a bit confused: where exactly? Inside of use-package or
> >>>> package-vc? Either way, I think raising an error when the user inputs
> >>>> nonsense is totally justified—I'd just like that error to be
> >>>> understandable as quickly as possible.
> >>>
> >>> I was thinking that package-vc should emit an error, but that
> >>> use-package could emit a warning, in case a new keyword is added to
> >>> package-vc specifications but hasn't yet been added to the use-package
> >>> layer -- mainly because I don't use the latter and am not that familiar
> >>> with the code.
> >>
> >> On first thought this sounds good, but I think one would end up having
> >> to sync the actual state of things in two places. What if package-vc.el
> >> had a place where it stored all available commands? Then the use-package
> >> integration could just use that. I think it would also be easier to have
> >> this codified somewhere, rather than having to check the manual every
> >> time.
> >
> > This sounds like a better solution, I can add a constant with all the
> > symbols that you can then use.  I am still a bit busy, so I cannot
> > promise when I'll manage to do so, but I'll ping you when the changes
> > have been pushed.
> 
> Can I perhaps bump this myself instead? :)
> 
> I'd really like to see this merged; if it helps move things along a
> bit quicker, I can also make the necessary changes to package-vc.el.

John, could you please review the patches and comment?

Message #46 received at 66567 <at> debbugs.gnu.org (full text, mbox):

From: John Wiegley <johnw <at> gnu.org>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: Tony Zorman <tonyzorman <at> mailbox.org>, philipk <at> posteo.net,
 66567 <at> debbugs.gnu.org
Subject: Re: bug#66567: [PATCH] use-package: Add ignored-files support to
 :vc keyword
Date: Fri, 03 May 2024 23:16:34 -0700

>>>>> Eli Zaretskii <eliz <at> gnu.org> writes:

>> I'd really like to see this merged; if it helps move things along a
>> bit quicker, I can also make the necessary changes to package-vc.el.

> John, could you please review the patches and comment?

Hi Eli, I’ve read through the proposed patches. What’s being proposed looks
good to me, except that I find it odd that it mentions two new keywords in the
ChangeLog that have nothing to do with the :vc keyword. Why conflate that into
this changeset? I don’t see those keywords mentioned anywhere else but the
valid keywords list, so is this for economy rather than creating two patches?

-- 
John Wiegley                  GPG fingerprint = 4710 CF98 AF9B 327B B80F
http://newartisans.com                          60E1 46C4 BD1A 7AC1 4BA2

Message #49 received at 66567 <at> debbugs.gnu.org (full text, mbox):

From: Tony Zorman <tonyzorman <at> mailbox.org>
To: John Wiegley <johnw <at> gnu.org>, Eli Zaretskii <eliz <at> gnu.org>
Cc: philipk <at> posteo.net, 66567 <at> debbugs.gnu.org
Subject: Re: bug#66567: [PATCH] use-package: Add ignored-files support to
 :vc keyword
Date: Tue, 14 May 2024 18:08:38 +0200

Hi,

On Fri, May 03 2024 23:16, John Wiegley wrote:
>>>>>> Eli Zaretskii <eliz <at> gnu.org> writes:
>
>>> I'd really like to see this merged; if it helps move things along a
>>> bit quicker, I can also make the necessary changes to package-vc.el.
>
>> John, could you please review the patches and comment?
>
> Hi Eli, I’ve read through the proposed patches. What’s being proposed looks
> good to me, except that I find it odd that it mentions two new keywords in the
> ChangeLog that have nothing to do with the :vc keyword. Why conflate that into
> this changeset? I don’t see those keywords mentioned anywhere else but the
> valid keywords list, so is this for economy rather than creating two patches?

I did attach two patches to the original message, separating these out.
Though you are right that perhaps I should have also created two
separate bugs for this. It's just something I noticed along the way and
figured I'd forget again if I didn't fix it immediately.

These keywords (I hope I haven't misunderstood you and you are referring
to :make and :shell-command) do, however, have something to do with :vc
in the sense that they are sub-keywords of it, much like :ignored-files
is.

  Tony

-- 
Tony Zorman | https://tony-zorman.com/

Message #52 received at 66567 <at> debbugs.gnu.org (full text, mbox):

From: "John Wiegley" <johnw <at> gnu.org>
To: "Tony Zorman" <tonyzorman <at> mailbox.org>, "Eli Zaretskii" <eliz <at> gnu.org>
Cc: Philip Kaludercic <philipk <at> posteo.net>, 66567 <at> debbugs.gnu.org
Subject: Re: bug#66567: [PATCH] use-package: Add ignored-files support to :vc
 keyword
Date: Tue, 14 May 2024 18:19:10 -0700

That all makes sense to me then. Looks good to go.

John

On Tue, May 14, 2024, at 9:08 AM, Tony Zorman wrote:
> Hi,
>
> On Fri, May 03 2024 23:16, John Wiegley wrote:
>>>>>>> Eli Zaretskii <eliz <at> gnu.org> writes:
>>
>>>> I'd really like to see this merged; if it helps move things along a
>>>> bit quicker, I can also make the necessary changes to package-vc.el.
>>
>>> John, could you please review the patches and comment?
>>
>> Hi Eli, I’ve read through the proposed patches. What’s being proposed looks
>> good to me, except that I find it odd that it mentions two new keywords in the
>> ChangeLog that have nothing to do with the :vc keyword. Why conflate that into
>> this changeset? I don’t see those keywords mentioned anywhere else but the
>> valid keywords list, so is this for economy rather than creating two patches?
>
> I did attach two patches to the original message, separating these out.
> Though you are right that perhaps I should have also created two
> separate bugs for this. It's just something I noticed along the way and
> figured I'd forget again if I didn't fix it immediately.
>
> These keywords (I hope I haven't misunderstood you and you are referring
> to :make and :shell-command) do, however, have something to do with :vc
> in the sense that they are sub-keywords of it, much like :ignored-files
> is.
>
>   Tony
>
> -- 
> Tony Zorman | https://tony-zorman.com/

Message #57 received at 66567-done <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: "John Wiegley" <johnw <at> gnu.org>
Cc: tonyzorman <at> mailbox.org, philipk <at> posteo.net, 66567-done <at> debbugs.gnu.org
Subject: Re: bug#66567: [PATCH] use-package: Add ignored-files support to :vc
 keyword
Date: Sat, 18 May 2024 13:32:49 +0300

> Date: Tue, 14 May 2024 18:19:10 -0700
> From: "John Wiegley" <johnw <at> gnu.org>
> Cc: "Philip Kaludercic" <philipk <at> posteo.net>, 66567 <at> debbugs.gnu.org
> 
> That all makes sense to me then. Looks good to go.

Thanks, installed on the master branch, and closing the bug.

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.