Re: How to contribute new package to GNU ELPA?

[Stefan Monnier <monnier@iro.umontreal.ca>]

Hi 

>> -;; Package-Requires: ((emacs "24.4") (cl-lib "0.5") (request "0.3.0"))
>> +;; Package-Requires: ((emacs "24.4") (request "0.3.0"))
>
> cl-lib should be required for `cl-function`.

cl-lib comes bundles with Emacs>=24.3, so your Emacs-24.4 dependency
already ensures cl-lib is available.

>>     ;; This file is part of GNU Emacs.
>>   @@ -28,13 +28,13 @@
>>     ;;; Commentary:
>>   -;;; This currently only works for Linux, not tested for Mac OS
>> X and Windows.
>> +;; This currently only works for Linux, not tested for Mac OS X and Windows.
>>   -;;; Kiwix installation
>> +;;;; Kiwix installation
>>   ;;
>>   ;; http://www.kiwix.org
>>   -;;; Config:
>> +;;;; Config:
>>   ;;
>>   ;; (use-package kiwix
>>   ;;   :ensure t
>> @@ -45,7 +45,7 @@
>>   ;;               kiwix-server-port 8080
>>   ;;               kiwix-default-library "wikipedia_zh_all_2015-11.zim"))
>>   -;;; Usage:
>> +;;;; Usage:
>
> Why use four `;` instead of three `;`? I search many packages, they all use
> three `;`. I don't know which one is the standard.

Because it's a sub-heading of "Commentary:" (e.g. we'd use five
semi-colons for subsubheadings).

>> +(if (featurep 'ivy) (require 'ivy))     ;FIXME: That's a no-op!
> What does the "no-op" mean? Because some user might have not installed ivy,
> so I added one condition to detect it here.

If (featurep 'ivy) is non-nil, then (require 'ivy) won't do anything.
I think your (featurep 'ivy) intended to test is Ivy is installed,
whereas it tests if Ivy is already loaded.  But in any case you don't
need any of that: if Ivy is installed, then `ivy-read` will be
auto-loaded, so the rest of your code will "just work" regardless if Ivy
is already loaded or not.

You might want to use something like:

    (defcustom kiwix-default-completing-read
      (cond ((fboundp 'ivy-read) 'ivy)
            ((fboundp 'helm) 'helm))

OTOH.

> Why deleted all `:group 'kiwix-mode`? If think correct, the :group is used
> by `customize-group`. So It should be necessary.

These are redundant because by default vars are placed in the group that
was last `defgroup`d.

>>   (defun kiwix-dir-detect ()
>>     "Detect Kiwix profile directory exist."
>> -  (let ((kiwix-dir (concat (getenv "HOME") "/.www.kiwix.org/kiwix")))
>> -    (if (file-accessible-directory-p kiwix-dir)
>> +  (let ((kiwix-dir "~/.www.kiwix.org/kiwix"))
>> +    (if (file-directory-p kiwix-dir)
> Use `file-accessible-directory-p` because also can detect folder
> permission. If user can't read folder, that's a problem too.

Then use something better (e.g. (and (file-directory-p kiwix-dir)
(file-readable-p kiwix-dir))), but (file-accessible-directory-p kiwix-dir)
returns non-nil when the directory is absent, in which case the user
can't read the directory either, so it's not testing what you want.

> I prefer to use `$HOME` environment variable, it should be more
> cross-platformed.

Fine by me.  I just thought it was an "obvious simplification".

Actually, it's the other way around: HOME is just an env-var and its
meaning is defined by the platform (it just happens to work fine with the
currently supported platforms), whereas "~/" is defined by Emacs so
we (Emacs maintainers) have to make sure it works on all platforms.  ;-)

>>         :success (cl-function
>> -                (lambda (&key data &allow-other-keys)
>> +                (lambda (&key _data &allow-other-keys) ;FIXME: Why not `&rest _'?
> For later success message output.

But that's another function, so it's unrelated, AFAICT.
So, I guess what you mean is that you put it for documentation purposes,
to remind the reader what args are passed to the success function (and
ignored in this particular success function).
FWIW, I find this to be a perfectly good reason (even thought it costs
a bit extra because the `cl-lib` will actually "work" to extract the `data`
argument even though it's not used).

>> -(defun kiwix-at-point (&optional interactively)
>> +(defun kiwix-at-point ()
>>     "Search for the symbol at point with `kiwix-query'.
>>     Or When prefix argument `INTERACTIVELY' specified, then prompt
>>   for query string and library interactively."
>> -  (interactive "P")
>> +  (interactive)
> This is necessary for later command definition `kiwix-at-point-interactive`.

No it's not.  What this did is add an `interactively` argument and pass
it the value of `current-prefix-arg`.  Since you don't use the variable
`interactively`, you don't need that: the variable `current-prefix-arg`
is not affected by the above changes.

This said, maybe I'm missing something: I don't see any place where you
implement the "When prefix argument `INTERACTIVELY' specified" test,
instead it seems your code just always "prompts for query string and
library interactively".

More importantly: what do you want to do about `request`?


        Stefan