Re: How to contribute new package to GNU ELPA?

[stardiviner <numbchild@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 6143 bytes --]

Thanks for your detailed answering. @Stefan Monnier
<monnier@iro.umontreal.ca>  It's really helpful! I tried to give up after
first email. :smile:

[stardiviner]           <Hack this world!>      GPG key ID: 47C32433
IRC(freeenode): stardiviner                     Twitter:  @numbchild
Key fingerprint = 9BAA 92BC CDDD B9EF 3B36  CB99 B8C4 B8E5 47C3 2433
Blog: http://stardiviner.github.io/


On Sat, Dec 19, 2020 at 11:35 PM Stefan Monnier <monnier@iro.umontreal.ca>
wrote:

> 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.
>
Applied.

>
> >>     ;; 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.
>
Applied.

>
> 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.
>
> Applied.

> > 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.  ;-)
>
Applied.

>
> >>         :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).
>
Hmm, let's keep this. I just applied "_data" to hint 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".
>

Applied too. I removed command `kiwix-at-point-interactive`.

> More importantly: what do you want to do about `request`?
>
> About this, I really suggest ELPA can include it. Because I hate to write
more complex url requests with `url.el`.
If someone want to adopt kiwix.el request to use `url.el`. I also can't
accept it. Because I still need to write code on it.
Also as you said, a wrapper on `url.el` is good for developer. Emacs will
need it. Really helpful.

At the end, thanks for your patient kind help. Thanks very much!

>
>         Stefan
>
>

[-- Attachment #2: Type: text/html, Size: 10306 bytes --]