[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: How to contribute new package to GNU ELPA?

From: stardiviner
Subject: Re: How to contribute new package to GNU ELPA?
Date: Sat, 19 Dec 2020 15:08:59 +0800
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.6.0

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" 

    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?


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 ""; kiwix-server-port)
    "Specify Kiwix server URL."
-  :type 'string
-  :group 'kiwix-mode)
+  :type 'string)
(defcustom kiwix-server-command
@@ -90,44 +87,40 @@
     ((string-equal system-type "windows-nt")
      (warn "You need to specify Windows Kiwix path. And send a PR to my 
    "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.

-      (warn "ERROR: Kiwix profile directory \".www.kiwix.org/kiwix\" is not 
+      (warn "ERROR: Kiwix profile directory \"~/.www.kiwix.org/kiwix\" is not 
+      nil)))
(defcustom kiwix-default-data-profile-name
    (when (kiwix-dir-detect)
-    (car (directory-files
-          (concat (getenv "HOME") "/.www.kiwix.org/kiwix") nil 
+    (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/" 
+    (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 
    "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 
    :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)
  (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)))))
-(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)
    (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.

reply via email to

[Prev in Thread] Current Thread [Next in Thread]