bug#60418: [PATCH] Add :vc keyword to use-package

[Philip Kaludercic]

Tony Zorman <soliditsallgood@mailbox.org> writes:

> On Sat, Apr 22 2023 11:32, Philip Kaludercic wrote:
>> (Sorry for all the back and forth, I hope you understand that this is
>> just to get stuff right and not for my own sake of being pedantic.  If
>> you don't have the time, I can also make the changes I'd just like to
>> hear your opinions.)
>
> No, I totally get it. Getting this stuff right the first time is much
> easier than going back and trying to fix it afterwards.

OK, great.  I just didn't want to make this seem like a ritual GNU thing ^^.

> I agree with most of your suggestions, so I'm just going to answer those
> where I have questions/comments.
>
>>> +(ert-deftest use-package-test/:vc-1 ()
>>> +  (match-expansion
>>> +   (use-package foo :vc (:url "bar"))
>>> +   '(use-package-vc-install '(foo (:url "bar") :last-release) nil)))
>>> +
>>> +(ert-deftest use-package-test/:vc-2 ()
>>> +  (match-expansion
>>> +   (use-package foo
>>> +     :vc (baz . (:url "baz" :vc-backend "Git"
>>> +                 :main-file qux.el :rev "rev-string")))
>>> +   `(use-package-vc-install '(baz
>>> +                              (:url "baz" :vc-backend Git :main-file 
>>> "qux.el")
>>> +                              "rev-string")
>>> +                            nil)))
>>> +
>>> +(ert-deftest use-package-test/:vc-3 ()
>>> +  (match-expansion
>>> +   (use-package foo :vc (bar . "baz"))
>>> +   `(use-package-vc-install '(bar "baz") nil)))
>>> +
>>> +(ert-deftest use-package-test/:vc-4 ()
>>> +  (match-expansion
>>> +   (use-package foo :vc (bar . (:url "baz" :rev :head)))
>>> +   '(use-package-vc-install '(bar (:url "baz") nil) nil)))
>>> +
>>> +(ert-deftest use-package-test/:vc-5 ()
>>> +  (let ((load-path? '(pred (apply-partially
>>> +                            #'string=
>>> +                            (expand-file-name "bar" 
>>> user-emacs-directory)))))
>>> +    (match-expansion
>>> +     (use-package foo :vc other-name :load-path "bar")
>>> +     `(progn (eval-and-compile
>>> +               (add-to-list 'load-path ,load-path?))
>>> +             (use-package-vc-install '(other-name) ,load-path?)))))
>>> +
>>> +(ert-deftest use-package-test-normalize/:vc ()
>>> +  (should (equal '(foo "version-string")
>>> +                 (use-package-normalize/:vc 'foo :vc '("version-string"))))
>>> +  (should (equal '(bar "version-string")
>>> +                 (use-package-normalize/:vc 'foo :vc '((bar . 
>>> "version-string")))))
>>> +  (should (equal '(foo (:url "bar") "baz")
>>> +                 (use-package-normalize/:vc 'foo :vc '((:url "bar" :rev 
>>> "baz")))))
>>> +  (should (equal '(foo)
>>> +                 (use-package-normalize/:vc 'foo :vc '(t))))
>>> +  (should (equal '(foo)
>>> +                 (use-package-normalize/:vc 'foo :vc nil)))
>>> +  (should (equal '(bar)
>>> +                 (use-package-normalize/:vc 'foo :vc '(bar)))))
>>
>> On my machine, these tests appear to fail?  Also, it would probably be
>> useful to document them (at leat the ...-1 to ...-5 ones) so that a
>> failure can be more easily understood.
>
> They don't fail for me—maybe I'm running the tests wrong? So far, I've
> done so interactively by evaluating the whole buffer and then running
> `M-x ert'. The -1…-5 tests just test the macro expansion, so e.g.
>
>     (ert-deftest use-package-test/:vc-1 ()
>       (match-expansion
>        (use-package foo :vc (:url "bar"))
>        '(use-package-vc-install '(foo (:url "bar") :last-release) nil)))
>
> would test if the first argument to `match-expansion' actually expands
> into the second one or not (which I guess explains why the error
> messages are a bit hard to understand).   

But that isn't the case, or am I mistaken?  The use-package form expands
to a lot mroe than just (use-package-vc-install ...)?

>>>                                It accepts the same arguments as
>>> +@code{package-vc-selected-packages} (@pxref{Specifying Package
>>> +Sources,,, emacs, GNU Emacs Manual}), except that a name need not
>>
>> This link does not appear to work.  I believe you can only link to a
>> node, and this is just a section.  Also, referring to the variable has
>> become an unnecessary redirection, since the variable documentation now
>> refers to the manual.
>
> Oh, I see. I have to admit that info links are still a bit of a mystery
> to me; sorry. So should I just link to the closest node, which would be
> "Fetching Package Sources" again, here?

You should be able to test this yourself by compiling Emacs and then
opening the manual to see how it was compiled.

>>> +For example,
>>> +
>>> +@lisp
>>> +@group
>>> +(use-package foo
>>> +  :vc (:url "https://bar.com/foo"; :rev :head))
>>> +@end group
>>> +@end lisp
>>
>> I think it would make sense to use the same examples as the Emacs manual
>> has been updated to use (BBDB) so that users can directly try out the
>> code.
>
> Sorry, I'm not sure I follow: the same examples as what exactly? Or do
> you just mean an actual package that users could try out when executing
> this code, instead of a dummy one?

Sorry, I should have been more explicit here.  I am referring to the
changes in 6e6e8b5c974202d2691c1971be66c1cb3788b7c1, where we moved some
of the documentation from package-vc.el to the Emacs manual.  Part of
the change included the addition of some package specification examples,
that I think would be a good fit here as well.

>>> +would try -- by invoking @code{package-vc-install} -- to install the
>>> +latest commit of the package @code{foo} from the specified remote.
>>> +
>>> +This can also be used for local packages, by combining it with the
>>> +@code{:load-path} (@pxref{Load path}) keyword:
>>> +
>>> +@lisp
>>> +@group
>>> +(use-package foo
>>> +  :vc t
>>> +  :load-path "/path/to/foo/)
>>> +@end group
>>> +@end lisp
>>> +
>>> +The above dispatches to @code{package-vc-install-from-checkout}
>>> +(@pxref{Fetching Package Sources,,, emacs, GNU Emacs Manual}).
>>
>> Does this reference here really help?
>
> Perhaps not. I don't have any strong opinions about this, so if you
> prefer I can remove this.

I think it would read better.