Re: [Emacs-diffs] master b689b90: Package archives now have priorities.

[Artur Malabarba <bruce.connor.am@gmail.com>]

>>> +(defun package--add-to-alist (pkg-desc alist)
>>> +  "Add PKG-DESC to ALIST.
>>> +
>>> +Packages are grouped by name. The package descriptions are sorted
>>> +by version number."
>>> +  (let* ((name (package-desc-name pkg-desc))
>>> +         (priority-version (package-desc-priority-version pkg-desc))
>>> +         (existing-packages (assq name alist)))
>>> +    (if (not existing-packages)
>>> +        (cons (list name pkg-desc)
>> This list should be a cons, probably why the test is failing.
>
> Why should this be a cons? The alist maps package names to ordered
> package descriptors – I guess (cons name (list pkg-desc)) would be
> clearer in intent.

Reading your code again, I see there are places where this `list' is
correct, but there's also one point where I think it's wrong.
Note the following diff section. It used to push `(cons
(package-desc-name pkg-desc) pkg-desc)', and now it uses
`package--add-to-alist' which pushes a list instead of a cons. Do you
see what I mean?

(defun package--download-one-archive (archive file)
   "Retrieve an archive file FILE from ARCHIVE, and cache it.
 ARCHIVE should be a cons cell of the form (NAME . LOCATION),
@@ -1991,18 +2035,18 @@ If optional arg BUTTON is non-nil, describe
its associated package."
       ;; ENTRY is (PKG-DESC [NAME VERSION STATUS DOC])
       (let ((pkg-desc (car entry))
            (status (aref (cadr entry) 2)))
-       (cond ((member status '("installed" "unsigned"))
-              (push pkg-desc installed))
-             ((member status '("available" "new"))
-              (push (cons (package-desc-name pkg-desc) pkg-desc)
-                     available)))))
+        (cond ((member status '("installed" "unsigned"))
+               (push pkg-desc installed))
+              ((member status '("available" "new"))
+               (setq available (package--add-to-alist pkg-desc available))))))

Maybe the solution is not to change `package--add-to-alist' but simply
to not use it here.


I also have a very tiny peeve here. Could we name this function
`package--alist-append' or something like this?

It's just that `add-to-alist' really reminds me of `add-to-list',
which has a very different effect (the list variable is the first
argument, it's referenced by name instead of value, and it's changed
destructively).
If you don't agree I won't push it. :-)