Re: [PATCH] Re: package.el changes before the feature freeze

[Chong Yidong]

In general, I find this patch very difficult to review.  As just one
example, you made a big change to `list-packages' by making it no longer
call package-read-all-archive-contents, but there is no justification or
explanation given, and appears to have nothing to do with the defstruct
cleanup.

Could you break it up into several different patches, each doing one
thing?

Here are some comments from a quick skim through the patch:

> -;; Version: 1.0
> +;; Version: 1.5

Why the jump of 0.4 versions?

> -;; GNU Emacs is free software: you can redistribute it and/or modify
> +;; GNU Emacs is free software; you can redistribute it and/or modify

Please get rid of such differences, they make the patch harder to read.

> -                :value-type (string :tag "URL or directory name"))
> +             :value-type (string :tag "URL or directory name"))

Likewise, please get rid of whitespace differences.

> +;; Translations for the old versions of package-desc-* substitutions.
> +(defsubst package-old-desc-vers (desc)
> +  "Extract version from an old-style package description vector."
> +  (aref desc 0))
> ...
> +(defvar package-builtins-newified nil

Please get rid of these functions and their callers, and all the newify
stuff.  Instead, change `finder-compile-keywords' in finder.el to use
the defstruct format and include it in the patch.

>      (cond ((string= sA sB)
>          (package-menu--name-predicate A B))
> -       ((string= sA "new") t)
> -       ((string= sB "new") nil)
> -       ((string= sA "available") t)
> +       ((string= sA  "available") t)
>         ((string= sB "available") nil)

Why did you get rid of the "new" status?  Also, you added more
gratuitous whitespace differences to this function.

> -(defun package-menu-mark-unmark (&optional _num)
> +(defun package-menu-mark-unmark (&optional num)

Why?