From: Daniel Hackney <dan@haxney.org>
To: Stefan Monnier <monnier@iro.umontreal.ca>
Cc: Chong Yidong <cyd@gnu.org>, emacs-devel@gnu.org
Subject: Re: [PATCH] Re: package.el changes before the feature freeze
Date: Mon, 8 Oct 2012 15:16:36 -0400 [thread overview]
Message-ID: <CAMqXDZvvhNfLpvDXAK5kcWfMTKdZg7Y2mYoF5TSVWEJcmtzrnA@mail.gmail.com> (raw)
In-Reply-To: <jwvzk401jk4.fsf-monnier+emacs@gnu.org>
Stefan Monnier <monnier@iro.umontreal.ca> 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.
next prev parent reply other threads:[~2012-10-08 19:16 UTC|newest]
Thread overview: 20+ messages / expand[flat|nested] mbox.gz Atom feed top
2012-09-30 16:58 package.el changes before the feature freeze Daniel Hackney
2012-09-30 20:21 ` Stefan Monnier
2012-09-30 23:50 ` Daniel Hackney
2012-10-01 1:21 ` Stefan Monnier
2012-10-01 3:11 ` Chong Yidong
2012-10-03 0:33 ` Daniel Hackney
2012-10-03 22:41 ` [PATCH] " Daniel Hackney
2012-10-04 8:16 ` Chong Yidong
2012-10-05 23:13 ` Daniel Hackney
2012-10-06 1:38 ` Stefan Monnier
2012-10-08 2:32 ` Chong Yidong
2012-10-08 19:16 ` Daniel Hackney [this message]
2012-10-08 19:50 ` Stefan Monnier
2012-10-09 1:11 ` Daniel Hackney
2012-10-09 6:48 ` Stefan Monnier
2012-10-09 17:07 ` Daniel Hackney
2012-10-09 17:39 ` Stefan Monnier
2012-10-09 21:39 ` Daniel Hackney
2012-10-09 22:25 ` Glenn Morris
2012-10-12 3:58 ` Chong Yidong
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=CAMqXDZvvhNfLpvDXAK5kcWfMTKdZg7Y2mYoF5TSVWEJcmtzrnA@mail.gmail.com \
--to=dan@haxney.org \
--cc=cyd@gnu.org \
--cc=emacs-devel@gnu.org \
--cc=monnier@iro.umontreal.ca \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this external index
https://git.savannah.gnu.org/cgit/emacs.git
https://git.savannah.gnu.org/cgit/emacs/org-mode.git
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.