bug#7756: 24.0.50; enhancements to package.el

[Richard Kim]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

>> @@ -338,9 +338,14 @@
>>       (pkg-file (expand-file-name
>>                  (concat (package-strip-version package) "-pkg")
>>                  pkg-dir)))
>> -    (when (and (file-directory-p pkg-dir)
>> -           (file-exists-p (concat pkg-file ".el")))
>> -      (load pkg-file nil t))))
>> +    ;; When one is creating a package and testing it out, it is easy
>> +    ;; to forget to add the -pkg.el file.  When that happens, it would
>> +    ;; be useful to provide feedback to the user rather than silently
>> +    ;; failing.  That is what (error ...) below is for.
>> +    (when (file-directory-p pkg-dir)
>> +      (if (file-exists-p (concat pkg-file ".el"))
>> +          (load pkg-file nil t)
>> +        (error "'%s' file is missing!")))))
>
> I agree with the intention, but I wonder: why do we pass the `noerror'
> parameter to `load' in the first place?

Hi Stefan,

Thanks for looking into this.
I was focused in adding the error message and neglected to re-examine
the arguments to `load', i.e., I just kept the existing code.  I suppose
it was used before my proposed change to prevent signalling error if the
file was not found.  With the proposed change, since we check for the
existence of the file prior to calling `load', I suppose it is
appropriate to no longer pass `noerror'.  Good catch.  Also I'm sure you
have noticed that I forgot to add `pkg-file' argument to (error ...)
expression above.

>> @@ -569,8 +574,16 @@
>>  (defun package-unpack (name version)
>>    (let ((pkg-dir (expand-file-name (concat (symbol-name name) "-" version)
>>                                 package-user-dir)))
>> +    ;; Delete the package directory if it exists already.  This may
>> +    ;; not be useful to the end users.  However this is extremely
>> +    ;; important for package developers as they experiment with which
>> +    ;; files to include in a package.  If a file is removed from one
>> +    ;; iteration to the next, then the presence of the unwanted elisp
>> +    ;; file could cause problems by polluting the generated autoload
>> +    ;; file.
>> +    (if (file-directory-p pkg-dir)
>> +        (delete-directory pkg-dir 'recursive))
>
> Here, I also agree with the intention, but I'm a little uneasy blindly
> removing a whole subdirectory without warning: I'd add a confirmation prompt.

I agree with your concern.  A confirmation prompt seems to be a good
idea.