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

[Daniel Hackney]

Stefan Monnier <address@hidden> 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.