[Top][All Lists]

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

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

From: Chong Yidong
Subject: Re: [PATCH] Re: package.el changes before the feature freeze
Date: Thu, 04 Oct 2012 16:16:51 +0800
User-agent: Gnus/5.13 (Gnus v5.13) Emacs/24.2.50 (gnu/linux)

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

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

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)


reply via email to

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