Re: How to contribute new package to GNU ELPA?

[stardiviner]

On 2020/12/19 下午2:22, Stefan Monnier wrote:
> stardiviner [2020-12-17 20:11:39] wrote:
> > As this issue https://github.com/stardiviner/kiwix.el/issues/3 suggested.
> > I would like to contribute my package kiwix.el to GNU ELPA.
> I've just started to look at it.  I have done some of the preliminary
> work, but there are some problems to solve:
>
> 1- The package uses `request` which is neither in Emacs nor in GNU ELPA.
> 2- The PNG images which are useful for the Homepage make the package
>     much larger for no good reason, so I think we should add
>     a `.elpaignore` file so as not to include those PNG imagines in the
>     package's tarball.
>
> 3- Even with `request`, compiling the package fails because
>     `org-kiwix.el` requirex `kiwix` and loading `kiwix` signal an error:
>
>        (file-missing "Opening directory" "Aucun fichier ou dossier de ce type" 
> "<HOMEDIR>/.www.kiwix.org/kiwix")
>
>     So, I think `file-accessible-directory-p` is not the function you
>     want to use.
>
> The patch below fixes some of those problems, but I still get warnings
> about a missing .www.kiwix... directory when I load the file, which can
> happen even if the user never intended to actually use any of that
> package's functionality: those warnings should be delayed to when we
> actually call some of the package's functions.
>
> The main problem, tho, is the `request` dependency.  There are two
> possible solutions to that: either we add `request` to GNU ELPA, or we
> rewrite the code so as not to use `request` (IIUC `request` is
> a reasonably thing wrapper above the `url` package, so it might not be
> too bad to do).  The best course is probably to add `request` to
> GNU ELPA.  Could you look into that, see how much work it would take to
> include `request` in GNU ELPA?
>
>
>          Stefan
>
>
> diff --git a/kiwix.el b/kiwix.el
> index 5b765b5d3c..377772ee2e 100644
> --- a/kiwix.el
> +++ b/kiwix.el
> @@ -1,5 +1,5 @@
> -;;; kiwix.el --- Searching offline Wikipedia through Kiwix.
> -;;; -*- coding: utf-8 -*-
> +;;; kiwix.el --- Searching offline Wikipedia through Kiwix.  -*- 
> lexical-binding: t; -*-
> +;; -*- coding: utf-8 -*-
>   
>   ;; Copyright (C) 2019-2020  Free Software Foundation, Inc.

lexical-binding added.

>   
> @@ -9,7 +9,7 @@
>   ;; URL: https://github.com/stardiviner/kiwix.el
>   ;; Created: 23th July 2016
>   ;; Version: 1.0.0
> -;; 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`.

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

>   ;;
>   ;; 1. [M-x kiwix-launch-server] to launch Kiwix server.
>   ;; 2. [M-x kiwix-at-point] to search the word under point or the region 
> selected string.
> @@ -58,7 +58,7 @@
>   (require 'subr-x)
>   (require 'thingatpt)
>   (require 'json)
> -(if (featurep 'ivy) (require 'ivy))
> +(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.

>   
>   (defgroup kiwix-mode nil
>     "Kiwix customization options."
> @@ -67,19 +67,16 @@
>   (defcustom kiwix-server-use-docker nil
>     "Using Docker container for kiwix-serve or not?"
>     :type 'boolean
> -  :safe #'booleanp
> -  :group 'kiwix-mode)
> +  :safe #'booleanp)
>   
>   (defcustom kiwix-server-port 8000
>     "Specify default kiwix-serve server port."
>     :type 'number
> -  :safe #'numberp
> -  :group 'kiwix-mode)
> +  :safe #'numberp)
>   
>   (defcustom kiwix-server-url (format "http://127.0.0.1:%s"; kiwix-server-port)
>     "Specify Kiwix server URL."
> -  :type 'string
> -  :group 'kiwix-mode)
> +  :type 'string)
>   
>   (defcustom kiwix-server-command
>     (cond
> @@ -90,44 +87,40 @@
>      ((string-equal system-type "windows-nt")
>       (warn "You need to specify Windows Kiwix path. And send a PR to my 
> repo.")))
>     "Specify kiwix server command."
> -  :type 'string
> -  :group 'kiwix-mode)
> +  :type 'string)
>   

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

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

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

>           kiwix-dir
> -      (warn "ERROR: Kiwix profile directory \".www.kiwix.org/kiwix\" is not 
> accessible."))))
> +      (warn "ERROR: Kiwix profile directory \"~/.www.kiwix.org/kiwix\" is not 
> accessible.")
> +      nil)))
Applied
>   
>   (defcustom kiwix-default-data-profile-name
>     (when (kiwix-dir-detect)
> -    (car (directory-files
> -          (concat (getenv "HOME") "/.www.kiwix.org/kiwix") nil 
> ".*\\.default")))
> +    (car (directory-files "~/.www.kiwix.org/kiwix" nil ".*\\.default")))
Same as upper.
>     "Specify the default Kiwix data profile path."
> -  :type 'string
> -  :group 'kiwix-mode)
> +  :type 'string)
>   
>   (defcustom kiwix-default-data-path
>     (when (kiwix-dir-detect)
> -    (concat (getenv "HOME") "/.www.kiwix.org/kiwix/" 
> kiwix-default-data-profile-name))
> +    (expand-file-name kiwix-default-data-profile-name
> +                      "~/.www.kiwix.org/kiwix/"))
>     "Specify the default Kiwix data path."
>     :type 'string
> -  :safe #'stringp
> -  :group 'kiwix-mode)
> +  :safe #'stringp)
>   
Same as upper.
>   (defcustom kiwix-default-library-path (file-name-directory
>                                          (concat kiwix-default-data-path 
> "/data/library/library.xml"))
>     "Kiwix libraries path."
>     :type 'string
> -  :safe #'stringp
> -  :group 'kiwix-mode)
> +  :safe #'stringp)
>   
>   (defcustom kiwix-default-completing-read 'ivy
>     "Kiwix default completion frontend. Currently Ivy ('ivy) and Helm ('helm) both 
> supported."
>     :type 'symbol
> -  :safe #'symbolp
> -  :group 'kiwix-mode)
> +  :safe #'symbolp)
>   
>   (defcustom kiwix-default-browser-function browse-url-browser-function
>     "Set default browser for open kiwix query result URL."
> @@ -139,8 +132,7 @@
>             (const :tag "Google Chrome web browser" browse-url-chrome)
>             (const :tag "Conkeror web browser" browse-url-conkeror)
>             (const :tag "xwidget browser" xwidget-webkit-browse-url))
> -  :safe #'symbolp
> -  :group 'kiwix-mode)
> +  :safe #'symbolp)
>   
>   ;;;###autoload
>   (defun kiwix--get-library-name (file)
> @@ -179,19 +171,16 @@ Like in function `kiwix-ajax-search-hints'.")
>   (defcustom kiwix-default-library "wikipedia_en_all.zim"
>     "The default kiwix library when library fragment in link not specified."
>     :type 'string
> -  :safe #'stringp
> -  :group 'kiwix-mode)
> +  :safe #'stringp)
>   
>   (defcustom kiwix-search-interactively t
>     "`kiwix-at-point' search interactively."
>     :type 'boolean
> -  :safe #'booleanp
> -  :group 'kiwix-mode)
> +  :safe #'booleanp)
>   
>   (defcustom kiwix-mode-prefix nil
>     "Specify kiwix-mode keybinding prefix before loading."
> -  :type 'kbd
> -  :group 'kiwix-mode)
> +  :type 'kbd)
>   
Same as upper.
>   ;; update kiwix server url and port
>   (defun kiwix-server-url-update ()
> @@ -258,7 +247,7 @@ Like in function `kiwix-ajax-search-hints'.")
>                   (when (string-equal (cdr error-thrown) "exited abnormally with 
> code 7\n")
>                     (warn "kiwix.el failed to connect to host. exited abnormally 
> with status code: 7."))))
>         :success (cl-function
> -                (lambda (&key data &allow-other-keys)
> +                (lambda (&key _data &allow-other-keys) ;FIXME: Why not `&rest 
> _'?
For later success message output.
>                     (setq kiwix-server-available? t)))
>         :status-code '((404 . (lambda (&rest _) (message (format "Endpoint %s does 
> not exist." url))))
>                        (500 . (lambda (&rest _) (message (format "Error from  
> %s." url))))))))
> @@ -290,12 +279,12 @@ list and return a list result."
>             (mapcar 'cdar data)))))
>   
>   ;;;###autoload
> -(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`.
>     (unless (kiwix-ping-server)
>       (kiwix-launch-server))
>     (if kiwix-server-available?
> @@ -368,7 +357,6 @@ for query string and library interactively."
>     :require 'kiwix-mode
>     :init-value nil
>     :lighter " Kiwix"
> -  :group 'kiwix-mode
>     :keymap kiwix-mode-map
>     (if kiwix-mode (kiwix-mode-enable) (kiwix-mode-disable)))
>   
Same as upper.