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