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

[Tony Zorman]

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.

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

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

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

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

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