GNU bug report logs - #37410
[PATCH] Several doc fixes in package.el

Previous Next

Full log

View this message in rfc822 format

From: Stefan Kangas <stefan <at> marxist.se>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 37410 <at> debbugs.gnu.org
Subject: bug#37410: [PATCH] Several doc fixes in package.el
Date: Sun, 15 Sep 2019 21:56:48 +0200

[Message part 1 (text/plain, inline)]

Eli Zaretskii <eliz <at> gnu.org> writes:

>> From: Stefan Kangas <stefan <at> marxist.se>
>> Date: Sun, 15 Sep 2019 18:01:17 +0200
>>
>> Seeing as the documentation in package.el leaves much to be desired, I
>> spent some time adding doc strings and fixing checkdoc and stylistic
>> errors.  I've attached a patch with my results, which should improve
>> the situation a little bit at least.
>
> Thanks for working on this.

Thanks for your detailed review.  I have attached an updated patch.

>>  (defun package-install-from-buffer ()
>> -  "Install a package from the current buffer.
>> +  "Install package from current buffer.
>
> Why this change?

Reverted that.

>>  ;;;###autoload
>>  (defun package-install-file (file)
>> -  "Install a package from a file.
>> +  "Install package from FILE.
>
> And this?

I think the original is wordy for no reason, and imprecise: We do not
install just "a package" in general, but specifically the package
pointed to by FILE.  But if I'm the only one who feels that the terse
version is better, I'm willing to concede that point.

>>  (defun describe-package-1 (pkg)
>> +  "Insert package description of PKG at point.
>> +Helper function for `describe-package'."
>
> The "at point" here is ambiguous: does it mean "insert at point" or
> "PKG at point"?

Changed that to:

    Insert the package description for PKG.

>>  (defun package-install-button-action (button)
>> +  "Run `package-install' on package defined by BUTTON.
>
> Can a package really be defined by a button?

Changed that to:

    Run `package-install' on the package BUTTON points to.

>>  (defun package-keyword-button-action (button)
>> +  "Show *Packages* buffer filtered by keyword from BUTTON label.
>
> *Packages* should be in double quotes.
>
> I generally find this sentence confusing: what do you mean by "keyword
> from BUTTON label"?

Changed that to:

+  "Show filtered \"*Packages*\" buffer for BUTTON.
+The buffer is filtered by the `package-keyword' property of BUTTON.

>> +(defun package-make-button (text &rest properties)
>> +  "Insert button labelled TEXT with button PROPERTIES at point.
>                     ^^^^^^^^
> "labeled"

Right, I used the British spelling by mistake.  Fixed.

>>  (defun package--print-email-button (name)
>> +  "Insert a button to email NAME at point.
>
> "To email NAME" is confusing.  I'd suggest to rename it ADDRESSEE.
> "Insert a button to email" is also confusing.  Is this alternative
> correct?

"Insert a button" is from the doc string of insert-text-button (for
which this is a wrapper) which says:

    "Insert a button with the label LABEL.#

>   Insert a button whose action will send email to ADDRESSEE.

Better, but I changed it to RECIPIENT instead of ADDRESSEE.

>>  (defvar package--emacs-version-list (version-to-list emacs-version)
>> -  "`emacs-version', as a list.")
>> +  "Variable `emacs-version' as a list.")
>
> "The value of `emacs-version', as a list."

Fixed.  (FWIW, I don't know what the point of this defvar is -- it's
only used once as far as I can tell.  Maybe it should just be removed.)

Best regards,
Stefan Kangas

[0001-Several-doc-fixes-in-package.el.patch (text/x-patch, attachment)]

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.