From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.org!not-for-mail From: Chong Yidong Newsgroups: gmane.emacs.devel Subject: Re: [PATCH] Re: package.el changes before the feature freeze Date: Thu, 04 Oct 2012 16:16:51 +0800 Message-ID: <87626qk5xo.fsf@gnu.org> References: <87ipau51jh.fsf@gnu.org> NNTP-Posting-Host: plane.gmane.org Mime-Version: 1.0 Content-Type: text/plain X-Trace: ger.gmane.org 1349402040 26346 80.91.229.3 (5 Oct 2012 01:54:00 GMT) X-Complaints-To: usenet@ger.gmane.org NNTP-Posting-Date: Fri, 5 Oct 2012 01:54:00 +0000 (UTC) Cc: emacs-devel@gnu.org To: Daniel Hackney Original-X-From: emacs-devel-bounces+ged-emacs-devel=m.gmane.org@gnu.org Fri Oct 05 03:54:06 2012 Return-path: Envelope-to: ged-emacs-devel@m.gmane.org Original-Received: from lists.gnu.org ([208.118.235.17]) by plane.gmane.org with esmtp (Exim 4.69) (envelope-from ) id 1TJx53-0005tv-Dr for ged-emacs-devel@m.gmane.org; Fri, 05 Oct 2012 03:51:57 +0200 Original-Received: from localhost ([::1]:43688 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TJgcF-0002Bv-Rz for ged-emacs-devel@m.gmane.org; Thu, 04 Oct 2012 04:17:07 -0400 Original-Received: from eggs.gnu.org ([208.118.235.92]:56076) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TJgcD-0002Bq-9S for emacs-devel@gnu.org; Thu, 04 Oct 2012 04:17:06 -0400 Original-Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1TJgc7-0001Eh-Ft for emacs-devel@gnu.org; Thu, 04 Oct 2012 04:17:05 -0400 Original-Received: from mail-pa0-f41.google.com ([209.85.220.41]:63753) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TJgc7-0001E0-9i for emacs-devel@gnu.org; Thu, 04 Oct 2012 04:16:59 -0400 Original-Received: by mail-pa0-f41.google.com with SMTP id fa10so305959pad.0 for ; Thu, 04 Oct 2012 01:16:57 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=sender:from:to:cc:subject:references:date:in-reply-to:message-id :user-agent:mime-version:content-type; bh=lx6MnwyBJuT2S4djiQEmfntfw9EWYkNWMyghL2NhckI=; b=NT9pnUH+gmIDCNUx4ovJyc9ZjmZ6Vtlw836P9ov+lqBbsBSShb9oElF2bPm3rOCIAI oo/D9d1m3KyZhkh/7M8t/bsdAyBcFPT40kYF1B+Fu0M78rKD406+a9l/e1wxK1pzrweH /QSgZw72kBVWozhSG1vszAhQVGRQRAtkSaiNnwwVWmt/dJysA3lUiAxkKuwW9hvC4d6Q ytb25xLQ2xMuhqLYcVeziBMDgNxiBE2fM2wPGFGB77i0xzh0Anx7LMOoaNQ1M0UY2QqI SB9pc//1KJXY8rRYG4fuf0cO89l6BgUnCBMQi3zU4OouREQ5FY2Pb6u9x2DBmCpUJXkt GbIA== Original-Received: by 10.68.201.104 with SMTP id jz8mr19977493pbc.141.1349338617853; Thu, 04 Oct 2012 01:16:57 -0700 (PDT) Original-Received: from ulysses ([155.69.17.122]) by mx.google.com with ESMTPS id n3sm2762657paz.25.2012.10.04.01.16.54 (version=SSLv3 cipher=OTHER); Thu, 04 Oct 2012 01:16:56 -0700 (PDT) In-Reply-To: (Daniel Hackney's message of "Wed, 3 Oct 2012 18:41:36 -0400") User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/24.2.50 (gnu/linux) X-detected-operating-system: by eggs.gnu.org: Genre and OS details not recognized. X-Received-From: 209.85.220.41 X-BeenThere: emacs-devel@gnu.org X-Mailman-Version: 2.1.14 Precedence: list List-Id: "Emacs development discussions." List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: emacs-devel-bounces+ged-emacs-devel=m.gmane.org@gnu.org Original-Sender: emacs-devel-bounces+ged-emacs-devel=m.gmane.org@gnu.org Xref: news.gmane.org gmane.emacs.devel:154064 Archived-At: 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?