bug#13291: The package description buffer needs an URL button

[Dmitry Gutov <dgutov@yandex.ru>]

On 11.03.2013 21:40, Stefan Monnier wrote:
>> And here's the updated patch for package.el, with saving the new metadata
>> to -pkg.el file when a single-file package is being installed, and with
>> support for it in `package-install-file'.
>
> Sorry for taking so long.  Here are some comments.

Not a problem, I rather expected some other developers familiar with 
this package to chime in.

>> +(require 'cl-lib)
>
> AFAICT, you only do that for cl-cddddr, but:
> - cl-cddddr only requires cl-lib at compile-time.
> - "nthcdr 4" is both shorter and faster.

Thanks, good to know. I wasn't aware that `nthcdr' is implemented in C.

>> -The vector DESC has the form [VERSION-LIST REQS DOCSTRING].
>> +The vector DESC has the form [VERSION-LIST REQS DOCSTRING META].
> [...]
>> +  META is a property list mapping metadata keywords to values.
>
> VERSION-LIST, REQS, and DOCSTRING are also metadata, so I'd call the new
> entry something like EXTRA or EXTRA-PROPS rather than META.

Sounds good.

> Also, I'd personally use an alist rather than a plist, tho it's largely
> a question of taste (I prefer alist because they have a bit more
> structure, which in turn lets you use things like mapcar, memq, dolist,
> ... on them).
> [ From a theoretical efficiency viewpoint they are also half as deep as
>    plists, so in an "ideal" PRAM world, they'd be about twice as fast,
>    although in reality I doubt there is any measurable difference.  ]

Hmm, I kinda assumed that the simpler structure would result in better, 
not worse, theoretical performance. Food for thought.

But I'm not really sure if we're ever going to want to iterate over the 
extra properties. Right now, where it's accessed, the change to alist 
will mostly mean going from

   (plist-get (package-desc-meta desc) :homepage)

to

   (cdr (assoc :homepage (package-desc-meta desc)))

which is not as nice.

>>   (defsubst package-desc-kind (desc)
>>     "Extract the kind of download from an archive package description vector."
>> +  (plist-get (package-desc-meta desc) :kind))
>> +
>> +(defsubst package-desc-meta (desc)
>> +  "Extract the metadata property list from a package description vector."
>>     (aref desc 3))
>
> Hmm... does that mean that the 4th field of each vector in
> `archive-contents' is changed from holding either `tar' or `single' to
> holding a plist?

Yes.

> Why not add a 5th field instead?

It actually has a 5th field already (containing the name of the 
repository). This is a bit complicated, because we also have 
`package-alist', its entries don't have the last two elements, and we 
want the homepage url to be available in both. But I guess we could push 
the new element before `kind' instead.

This may be moot now, now that we have the updated Daniel's 
defstruct-based rewrite, which uses struct accessor functions here, so 
adding another slot would be the way to go.

(Since it has a test suite, merging this patch on top of it should be 
easier than going the other way around).

>>   (defun define-package (name-string version-string
>>   				&optional docstring requirements
>> -				&rest _extra-properties)
>> +				&rest extra-properties)
> [...]
>> -EXTRA-PROPERTIES is currently unused."
>> +EXTRA-PROPERTIES is a property list mapping additional metadata
>> +keywords (e.g. `:homepage') to values."
>
> I guess here I agree that a plist is more convenient for the
> package maintainer.

One of the things you haven't commented on is that the vectors in the 
archive-contents file are of variable length with this proposal.

Should

(cl-lib . [(0 2) nil "Prefixed!" single :homepage "http://foo"])

turn into

(cl-lib . [(0 2) nil "Prefixed!" single (:homepage "http://foo")])

or maybe

(cl-lib . [(0 2) nil "Prefixed!" single (:homepage . "http://foo")])

?