[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

From: Dmitry Gutov
Subject: bug#13291: The package description buffer needs an URL button
Date: Tue, 12 Mar 2013 15:49:36 +0400
User-agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:17.0) Gecko/20130307 Thunderbird/17.0.4

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


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


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.


(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";)])


reply via email to

[Prev in Thread] Current Thread [Next in Thread]