From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.org!not-for-mail From: Daniel Hackney Newsgroups: gmane.emacs.devel Subject: Re: [PATCH] Re: package.el changes before the feature freeze Date: Mon, 8 Oct 2012 15:16:36 -0400 Message-ID: References: <87ipau51jh.fsf@gnu.org> <87626qk5xo.fsf@gnu.org> NNTP-Posting-Host: plane.gmane.org Mime-Version: 1.0 Content-Type: text/plain; charset=UTF-8 X-Trace: ger.gmane.org 1349723826 22312 80.91.229.3 (8 Oct 2012 19:17:06 GMT) X-Complaints-To: usenet@ger.gmane.org NNTP-Posting-Date: Mon, 8 Oct 2012 19:17:06 +0000 (UTC) Cc: Chong Yidong , emacs-devel@gnu.org To: Stefan Monnier Original-X-From: emacs-devel-bounces+ged-emacs-devel=m.gmane.org@gnu.org Mon Oct 08 21:17:12 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 1TLIpD-0007lu-1X for ged-emacs-devel@m.gmane.org; Mon, 08 Oct 2012 21:17:11 +0200 Original-Received: from localhost ([::1]:45303 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TLIp6-0000Uu-UD for ged-emacs-devel@m.gmane.org; Mon, 08 Oct 2012 15:17:04 -0400 Original-Received: from eggs.gnu.org ([208.118.235.92]:56476) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TLIp4-0000UR-NY for emacs-devel@gnu.org; Mon, 08 Oct 2012 15:17:04 -0400 Original-Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1TLIp2-0001Lq-Ds for emacs-devel@gnu.org; Mon, 08 Oct 2012 15:17:02 -0400 Original-Received: from mail-ie0-f169.google.com ([209.85.223.169]:44430) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TLIp2-0001Ld-5c for emacs-devel@gnu.org; Mon, 08 Oct 2012 15:17:00 -0400 Original-Received: by mail-ie0-f169.google.com with SMTP id 10so12183885ied.0 for ; Mon, 08 Oct 2012 12:16:59 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=haxney.org; s=google; h=mime-version:in-reply-to:references:from:date:message-id:subject:to :cc:content-type; bh=dXGwytA4NCtHDPk4wcQ36Jsfy/DQ87poygLPC2pXhdM=; b=aOyRVXvWzFJW7w0b5elLBuRDvOkvfXFdaMgPFWmBSkt/x+ofVbE8+3mivj84Ooqj6f cUfRyikvDgYEaVY0x/frFwaJth4JrHofOLL00kK5d53rIQOWxPXl5plACvdWfyXkoUUs hkyOxyrWfqdhJXvjCZ6n3pwQbkfTt12tNN8Fc= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20120113; h=mime-version:in-reply-to:references:from:date:message-id:subject:to :cc:content-type:x-gm-message-state; bh=dXGwytA4NCtHDPk4wcQ36Jsfy/DQ87poygLPC2pXhdM=; b=G10mBaN66HCbef7H2tbEBbhyp2RDs1HAA8saEouO96XOYOkTpjVJoRSrUL5cy5SMUf 76PAMKOJMBYkurdLrQoJcEi7rcLq7lu3sJGLU3i5QavboF6FGDbHnrwBk6oYksOJ81GZ kuTXxa8r48E878kPykEBADJGOR/ZNVhV2Mh66XTOPLPCra5lvpLyYWZ8KuSFaZblGMjZ O0/yqQNGFgubkwqSnCL0v7GXhnL99SzUnJSNMztVmH0eJTFXA3CC2UMECYwsUBUkHseG unbnfkEsTAjakgOkxAb/LebAArCFX/3w1+VstIj18NwKChs9C1Dn6+tbNZXdUwD5vWpb hoKA== Original-Received: by 10.42.22.131 with SMTP id o3mr1751077icb.55.1349723817164; Mon, 08 Oct 2012 12:16:57 -0700 (PDT) Original-Received: by 10.64.101.197 with HTTP; Mon, 8 Oct 2012 12:16:36 -0700 (PDT) In-Reply-To: X-Gm-Message-State: ALoCoQlVxWvlkcG1o8PQDTatmd9sS7Xzs+rxBdWFP0bKsDyZIc8o6a+JyaMUa0X16XNrJcpy8FIi X-detected-operating-system: by eggs.gnu.org: Genre and OS details not recognized. X-Received-From: 209.85.223.169 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:154244 Archived-At: Stefan Monnier wrote: > I haven't read the whole patch, but here are some nitpicks. > The general idea looks fine, tho. We'd need a ChangeLog with that, > it should describe the changes that are neither cosmetic nor simple > adjustments to the use of defstruct. Okay. I'll take a look at existing ChangeLog entries and try to duplicate the style and format. >> +Slots: >> + >> +`:name' >> +Name of the package, as a symbol. >> + >> +`:version' >> +Version of the package, as a version list. >> + >> +`:summary' >> +Short description of the package, typically taken from the first >> +line of the file. >> + >> +`:reqs' >> +Requirements of the package. A list of (PACKAGE VERSION-LIST) >> +naming the dependent package and the minimum required version. >> + >> +`:kind' >> +The distribution format of the package. Currently, it is either >> +`single' or `tar'. >> + >> +`:archive' >> +The name of the archive (as a string) whence this package came." >> + >> + name >> + version >> + (summary "No description available.") >> + reqs >> + kind >> + archive) > > Nitpick: the fields of the struct (which you can call "slots" if you > prefer, of course) don't have a ":" in front of their name. The CL info pages refer to them as slots, so that's what I figured I'd use. There could be a better name for a package defstruct, but "field" "property" and "attribute" are overdone :) True. I was basing that off of the fact that calling `make-package-desc' requires passing in the slot names as `:'-prefixed symbols. I guess that's a different issue; I'll change them to the un-prefixed version. > [ I'd also prefer using fewer lines in the docstring, so the whole > definition can hopefully fit within a tall-but-split frame. ] I changed the slot documentation so that the name and the start of the description start on the same line. >> -(defun package-activate-1 (package pkg-vec) >> - (let* ((name (symbol-name package)) >> - (version-str (package-version-join (package-desc-vers pkg-vec))) >> +(defun package-activate-1 (pkg-desc) >> + (let* ((name (package-desc-name pkg-desc)) >> + (version-str (package-version-join (package-desc-version pkg-desc))) >> (pkg-dir (package--dir name version-str))) > > Hmm... `name' in the new code is now a symbol whereas it was a string in > the old code. Is that right? That's right. Like Chong said, what I'm calling `name' now is `assq'ed all over the place, so it's a matter of using `symbol-name' or `intern' in a bunch of places. Symbols feel more "canonical-y" to me. I agree that the slot name could definitely be improved. `name' does imply a string to me, but I think that it is good for the "primary key" of the alists to be a symbol. Something like `canonical-name' perhaps? `id' maybe? I'm not terribly attached to any particular slot name. >> - (load (expand-file-name (concat name "-autoloads") pkg-dir) nil t) >> + (load (expand-file-name (concat (symbol-name name) "-autoloads") >> pkg-dir) nil t) > > You can use (format "%s-autoloads" name) to make it work equally with > strings and symbols. I think the reason I used the `concat' was to reduce the changes the patch introduced. I'll switch to `format'. >> + (apply 'define-package-desc > > BTW,please stick to the "package-" prefix. This was intended to be related to `define-package'; it turns the `define-package' call into a `package-desc' struct. Since `define-package' is part of the externally-facing API, it cannot change. I suppose because `define-package' from the "foo-pkg.el" file isn't being `eval'd, there doesn't actually need to be a function named `define-package'. Do you want me to change that back to a call to `make-package-desc'? >> + name-string >> + version-string >> + summary >> + requirements >> + _extra-properties))) > > Obviously you haven't played with lexical-binding yet, but the "leading > underscore" is used to denote variables/arguments that are not used, so > the above use of _extra-properties indicates that it should be named > `extra-properties' instead. I haven't looked at lexical binding in earnest. Should I reference the non-prefixed form in the body of `define-package'? >> - (package-unpack name version)))) >> + (package-unpack (symbol-name name) version)))) > > All those make me wonder: do we need the `name' slot to be symbol? > Why not let it be a string? Actually, switching the `concat's to `format's means that there is much less of a need for explicit `symbol-name's. I'll see if I can get the functions passing around a symbol only >> + (make-package-desc :name name > > I know it's the default, but I also prefer not to use the "make-" prefix > and use "package-" as the prefix instead. Sure thing.