From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.io!.POSTED.blaine.gmane.org!not-for-mail From: stardiviner Newsgroups: gmane.emacs.devel Subject: Re: How to contribute new package to GNU ELPA? Date: Sun, 20 Dec 2020 10:12:43 +0800 Message-ID: References: <15c3cc00-f56e-6e52-2228-30817639315a@gmail.com> Mime-Version: 1.0 Content-Type: multipart/alternative; boundary="000000000000694dc005b6dbe23f" Injection-Info: ciao.gmane.io; posting-host="blaine.gmane.org:116.202.254.214"; logging-data="8663"; mail-complaints-to="usenet@ciao.gmane.io" Cc: Emacs developers To: Stefan Monnier Original-X-From: emacs-devel-bounces+ged-emacs-devel=m.gmane-mx.org@gnu.org Sun Dec 20 03:14:00 2020 Return-path: Envelope-to: ged-emacs-devel@m.gmane-mx.org Original-Received: from lists.gnu.org ([209.51.188.17]) by ciao.gmane.io with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1kqoER-00028W-KZ for ged-emacs-devel@m.gmane-mx.org; Sun, 20 Dec 2020 03:13:59 +0100 Original-Received: from localhost ([::1]:53082 helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1kqoEQ-0001jm-Kr for ged-emacs-devel@m.gmane-mx.org; Sat, 19 Dec 2020 21:13:58 -0500 Original-Received: from eggs.gnu.org ([2001:470:142:3::10]:48644) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1kqoDo-0001Fg-85 for emacs-devel@gnu.org; Sat, 19 Dec 2020 21:13:20 -0500 Original-Received: from mail-vs1-xe30.google.com ([2607:f8b0:4864:20::e30]:38436) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.90_1) (envelope-from ) id 1kqoDl-0002AY-Nb for emacs-devel@gnu.org; Sat, 19 Dec 2020 21:13:19 -0500 Original-Received: by mail-vs1-xe30.google.com with SMTP id z16so3555746vsp.5 for ; Sat, 19 Dec 2020 18:13:16 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=wgD5vmHnowQ/rMaOI9OyvoOJvDAo2MrOk+oPswhVGc0=; b=K5HuWFnnSOglgFOrO5qZH7jMpH5JGj96U3h5FvNrZ/mvncAu5j53VhwqR3iItiIIc6 5DrgMBy+Ox4GWW+3ve2bFX+789jkXFdNZd+lVg/kqhws7PkbJen5zHRPNLS0f7Ibu14N DwOxT2yhsIUDMsQ4sAMvPaMoesvKV7429anFKujOJDbPs359LN/tUgNTFCqbqKf/eMXL T4yOQr+e6Hi/IFRyvhrUJKzkZ+r9U4HDQ1Rch/lY7wl+rwk/AWIvxWx5GWg2aac2WLtF u4QKyaOrQh1nn7iUwk5IXhCM4O9qdITdLNKAI3WP4N0pOEq4guVB187hgLaiDRg+mSm/ VTYg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=wgD5vmHnowQ/rMaOI9OyvoOJvDAo2MrOk+oPswhVGc0=; b=Ond8fcAoHD3SUfv6gkmGn9oTycGOzzsaoMUHpDuOvnAsk6x7PQfEu2S5HOcb1YjNPh vreLkZpiPk42tJLLTjPT68k7XdhLhScqfz0qi/egm9WHi6hHvTXYn3Hh8eh9RubCIhIW 1JJUe8dmT5gxYOyRzxij02COyGO2UgWZ9qGkRSPAMFKfoYxWfCNbP2lr/LY+YZurbvGF lyq3HNAs4wYGiHt++smkAHHmptCZI9mP148lzGEUXO6H48Ok+Wggff0evg1EC5rGqqoA kJ3ZX5A8/154/z6duIkEXcP0xlE6Sm5iCxjgqybwHIrjRC7UeKvRJxRwypi/s9CqjzcD xcNw== X-Gm-Message-State: AOAM533GidW4rAB+Qc2sHiLI2TWTokgXGTLvXYGt03gb6lturrGy0rRB 3B01nNR6o6S1vDBdBv71eZHPovkRM6aSV7t/Uw== X-Google-Smtp-Source: ABdhPJzGkZInTiBEQYOjONiOihWtuGQ1k/am5UUzx+MNXIn9mDuyp71yeh05Bro/ffn1MwAhOX00J0RPffkvFeVh8cw= X-Received: by 2002:a67:2d8f:: with SMTP id t137mr10024490vst.28.1608430396196; Sat, 19 Dec 2020 18:13:16 -0800 (PST) In-Reply-To: Received-SPF: pass client-ip=2607:f8b0:4864:20::e30; envelope-from=numbchild@gmail.com; helo=mail-vs1-xe30.google.com X-Spam_score_int: -20 X-Spam_score: -2.1 X-Spam_bar: -- X-Spam_report: (-2.1 / 5.0 requ) BAYES_00=-1.9, DKIM_SIGNED=0.1, DKIM_VALID=-0.1, DKIM_VALID_AU=-0.1, DKIM_VALID_EF=-0.1, FREEMAIL_FROM=0.001, HTML_MESSAGE=0.001, RCVD_IN_DNSWL_NONE=-0.0001, SPF_HELO_NONE=0.001, SPF_PASS=-0.001 autolearn=ham autolearn_force=no X-Spam_action: no action X-BeenThere: emacs-devel@gnu.org X-Mailman-Version: 2.1.23 Precedence: list List-Id: "Emacs development discussions." List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: emacs-devel-bounces+ged-emacs-devel=m.gmane-mx.org@gnu.org Original-Sender: "Emacs-devel" Xref: news.gmane.io gmane.emacs.devel:261297 Archived-At: --000000000000694dc005b6dbe23f Content-Type: text/plain; charset="UTF-8" Thanks for your detailed answering. @Stefan Monnier It's really helpful! I tried to give up after first email. :smile: [stardiviner] 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 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 > > --000000000000694dc005b6dbe23f Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable
Thanks for your detailed answering. @Stefan Monnier=C2=A0 It's really help= ful! I tried to give up after first email. :smile:

[stardiviner]=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 <Hack this world!= >=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 GPG key ID: 47C32433
IRC(freeenode): = stardiviner =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 Twitter:=C2=A0 @numbchi= ld
Key fingerprint =3D 9BAA 92BC CDDD B9EF 3B36=C2=A0 CB99 B8C4 B8E5 47C= 3 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&= quot;) (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>=3D24.3, so your Emacs-24.4 dependency already ensures cl-lib is available.
Applied.

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

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

>> +(if (featurep 'ivy) (require 'ivy))=C2=A0 =C2=A0 =C2=A0;F= IXME: 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 any= thing.
I think your (featurep 'ivy) intended to test is Ivy is installed,
whereas it tests if Ivy is already loaded.=C2=A0 But in any case you don= 9;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:

=C2=A0 =C2=A0 (defcustom kiwix-default-completing-read
=C2=A0 =C2=A0 =C2=A0 (cond ((fboundp 'ivy-read) 'ivy)
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 ((fboundp 'helm) 'helm))<= br>
OTOH.

Applied.=C2=A0
> 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.

>>=C2=A0 =C2=A0(defun kiwix-dir-detect ()
>>=C2=A0 =C2=A0 =C2=A0"Detect Kiwix profile directory exist.&quo= t;
>> -=C2=A0 (let ((kiwix-dir (concat (getenv "HOME") "/= .www.kiwix.org/kiwix")))
>> -=C2=A0 =C2=A0 (if (file-accessible-directory-p kiwix-dir)
>> +=C2=A0 (let ((kiwix-dir "~/.www.kiwix.org/kiwix"))=
>> +=C2=A0 =C2=A0 (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)<= br> 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.=C2=A0 I just thought it was an "obvious simplification&quo= t;.

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

>>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0:success (cl-function
>> -=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 (lambda (= &key data &allow-other-keys)
>> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 (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&#= 39;s keep this. I just applied "_data" to hint it's not used.= =C2=A0

>> -(defun kiwix-at-point (&optional interactively)
>> +(defun kiwix-at-point ()
>>=C2=A0 =C2=A0 =C2=A0"Search for the symbol at point with `kiwi= x-query'.
>>=C2=A0 =C2=A0 =C2=A0Or When prefix argument `INTERACTIVELY' spe= cified, then prompt
>>=C2=A0 =C2=A0for query string and library interactively."
>> -=C2=A0 (interactive "P")
>> +=C2=A0 (interactive)
> This is necessary for later command definition `kiwix-at-point-interac= tive`.

No it's not.=C2=A0 What this did is add an `interactively` argument and= pass
it the value of `current-prefix-arg`.=C2=A0 Since you don't use the var= iable
`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".
=C2=A0
Applied too. I rem= oved command `kiwix-at-point-interactive`.=C2=A0
More importantly: what do you want to do about `request`?

About this, I really suggest ELPA can include it. Be= cause 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 wil= l need it. Really helpful.

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

=C2=A0 =C2=A0 =C2=A0 =C2=A0 Stefan

--000000000000694dc005b6dbe23f--