Package: emacs;
Reported by: Tony Zorman <tonyzorman <at> mailbox.org>
Date: Sun, 15 Oct 2023 18:00:02 UTC
Severity: wishlist
Tags: patch
Done: Eli Zaretskii <eliz <at> gnu.org>
Bug is archived. No further changes may be made.
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
GNU bug tracking system
Copyright (C) 1999 Darren O. Benham,
1997,2003 nCipher Corporation Ltd,
1994-97 Ian Jackson.