Re: [PATCH] Add pinyin-isearch to NonGNU ELPA
Anoncheg <[email protected]> Sat, 08 Aug 2026 18:49:26 +0000
| Newsgroups | gmane.emacs.devel |
|---|---|
| Message-ID | <[email protected]> |
Hello, please review the package for repository inclusion. A line to add to elpa-packages: ```lisp (pinyin-isearch :url "https://codeberg.org/Anoncheg/pinyin-isearch") ``` Anoncheg <[email protected]> writes: >> the code seems somewhat chaotic >> (which is also a reason I am interested in the LLM question above). > > Hello, > I am working hard to make this code less chaotic, and I don't use LLMs. > This package is old and has been actively used and well-tested for over two years. > >> I am wondering if it wouldn't make >> sense for you to wait a bit and polish the code before adding it to >> ELPA? > I don't have many social connections to advertise the package. > I need it on ELPA to make it public. > > Please use this line (without :readme and :ignored-files) to add package: > > (pinyin-isearch :url "https://codeberg.org/Anoncheg/pinyin-isearch") > > I would appreciate more critical feedback. Thank you for your help! > > > Philip Kaludercic <[email protected]> writes: > >> Vitaliy Chepelev <[email protected]> writes: >> >>> Hello, >>> >>> I'm submitting my Emacs package 'pinyin-isearch' for inclusion in NonGNU ELPA. >>> >>> Attached is a git patch adding my package to new branch `externals/pinyin-isearch`, and >>> a line to elpa-packages. >>> >>> It is AGPL-3+, that lives at >>> https://codeberg.org/Anoncheg/pinyin-isearch >>> for ongoing updates. >> >> First of all, please do not include an eval block in your .dir-locals >> that re-binds keys, even if this is just local. >> >> Next, there have been some questions about the usage of generative AI >> for packages, even in NonGNU ELPA. Can you disclose if you have used >> LLMs or anything similar, and to what capacity? >> >> I went over the code and left a few comments, questions and hints for >> you to consider: >> >> diff --git a/pinyin-isearch-chars.el b/pinyin-isearch-chars.el >> index 17e2634..d4394f9 100644 >> --- a/pinyin-isearch-chars.el >> +++ b/pinyin-isearch-chars.el >> @@ -57,7 +57,7 @@ >> >> (require 'pinyin-isearch-loaders) ; for pinyin-isearch-loaders--py-punct-rules >> >> -(require 'cl-extra) ; for `cl-some' >> +(require 'cl-lib) ; for `cl-some' >> >> (require 'subr-x) ; for 28.1 and `string-join' >> >> @@ -79,7 +79,6 @@ If there is undecoded letters at the end after dissasembling." >> :type 'boolean >> :group 'pinyin-isearch) >> >> - >> ;; ---------- loaded punct and concatenate: py + punct -------- >> (defvar pinyin-isearch-chars--py-rules nil >> "Rules in form: ((\"a\" \"阿啊呵腌嗄锕吖\") (\"ai\" \"爱哀挨碍埃癌艾唉矮哎皑蔼隘暧霭捱嗳瑷嫒锿嗌砹\")...") >> @@ -106,16 +105,17 @@ If there is undecoded letters at the end after dissasembling." >> "Create table that allow quickly find syllable by it's first letters. >> Argument RULES argument of funcion `quail-define-rules'." >> (let ((ss nil)) >> - (dolist ( r rules) >> + (dolist (r rules) >> (let ((rr (car r))) >> + ;; I think you could make this a lot more readable by using `cl-loop'. >> (dolist (l (number-sequence 1 (length rr))) >> (let* ((sub (substring (car r) 0 l)) >> (el (assoc-string sub ss)) >> (newl)) >> (when (not el) ;; el is nil >> - (dolist ( r rules) >> - (if (string-prefix-p sub (car r)) >> - (push (car r) newl))) >> + (dolist (r rules) >> + (when (string-prefix-p sub (car r)) >> + (push (car r) newl))) >> (setq ss (cons (list sub newl) ss))))))) >> ss)) >> >> @@ -136,9 +136,9 @@ Argument RULES argument of funcion `quail-define-rules'." >> "Interface to constant `pinyin-isearch-chars--first-syllable-letters'. >> For \"a\" we get (ao ang an ai a). >> Argument ST the begining letters of any syllable." >> - (let ((v (assoc-string st pinyin-isearch-chars--first-syllable-letters ))) >> + (let ((v (assoc-string st pinyin-isearch-chars--first-syllable-letters))) >> (if v >> - (let (( res (copy-tree (car (cdr v))))) >> + (let ((res (copy-tree (car (cdr v))))) >> ;; remove nv and lv from result >> (cond ((equal st "n") (setq res (remove "nv" res))) >> ((equal st "l") (setq res (remove "lv" res)))) >> @@ -220,7 +220,7 @@ Returns a list of list of syllable lists or >> ((and (not results) >> (not pinyin-isearch-strict) >> pinyin-isearch-chars-fallback >> - (> (length st) 0)) >> + st) >> (list (list (list (concat pinyin-isearch-chars--non-syllable-marker-string st))))) >> (t results)))) >> >> @@ -236,7 +236,7 @@ Argument LVAR dissasembled variants of characters for query." >> ;; add full string for fallback to latin if we have unconvertable characters at the end (marked) >> (if (and pinyin-isearch-chars-fallback >> (not pinyin-isearch-strict) >> - (or (> (length lvar) 1) (> (length (car lvar)) 1))) >> + (or (length> lvar 1) (length> (car lvar) 1))) >> (let ((la (car (car (last (car (last lvar))))))) >> (if (eq (elt la 0) pinyin-isearch-chars--non-syllable-marker-number) >> ;; add full string (marked) to result list as an another variant. >> @@ -253,7 +253,7 @@ Argument LVAR dissasembled variants of characters for query." >> (when (and (not (string-empty-p string)) >> pinyin-isearch-full-fallback >> (not pinyin-isearch-strict)) >> - (push (mapcar #'list (mapcar #'char-to-string (string-to-list string))) >> + (push (mapcar #'list (mapcar #'char-to-string string)) >> lvar)) >> lvar) >> >> @@ -284,8 +284,7 @@ Argument LVAR dissasembled variants of characters for query." >> ;; Faster variant: >> (defun pinyin-isearch-chars--is-here-p (orig-str sac) >> "Check that we have any leaf in SAC equal ORIG-STR." >> - (cl-some (lambda (x) (when (string-equal x orig-str) t)) >> - (flatten-tree sac))) >> + (cl-some orig-str (flatten-tree sac) :test #'string-equal)) >> >> ;; ;; Slower variant: >> ;; (require 'seq) >> @@ -333,25 +332,23 @@ Example: >> (let ((marker pinyin-isearch-chars--non-syllable-marker-number)) >> (mapconcat >> (lambda (inner) >> - (let* ((marked (delq nil (mapcar (lambda (s) >> - (if (and (> (length s) 0) (eq (elt s 0) marker)) >> - (substring s 1) >> - nil)) >> - inner))) >> - (unmarked (delq nil (mapcar (lambda (s) >> - (if (or (not (> (length s) 0)) >> - (not (eq (elt s 0) marker))) >> - s >> - nil)) >> - inner)))) >> + (let* ((marked (mapcan (lambda (s) >> + (and s (eq (elt s 0) marker) >> + (list (substring s 1)))) >> + inner)) >> + (unmarked (mapcan (lambda (s) >> + (and (or (not s) >> + (not (eq (elt s 0) marker))) >> + (list s))) >> + inner))) >> (cond >> ;; All marked: concat, no brackets >> ((and (> (length marked) 0) >> (= (length marked) (length inner))) >> (apply #'concat marked)) >> ;; Only one unmarked, single char: no bracket >> - ((and (= (length unmarked) 1) >> - (= (length (car unmarked)) 1)) >> + ((and (length= unmarked 1) >> + (length= (car unmarked) 1)) >> (concat (car unmarked) (apply #'concat marked))) >> ;; Otherwise: bracket unmarked, append marked >> (t >> @@ -411,8 +408,6 @@ including the original pinyin if `pinyin-isearch-full-fallback' is true. >> (defvar-local pinyin-isearch-chars--cached-full-fallback-flag nil >> "Non-nil means values saved for `pinyin-isearch-chars-regexp-function'.") >> >> - >> - >> (defun pinyin-isearch-chars-regexp-function (string &optional _lax) >> "Replacement for function `isearch-regexp-function'. >> Input - pinyin, output - regex for isearch. >> diff --git a/pinyin-isearch-loaders.el b/pinyin-isearch-loaders.el >> index bca8bc6..4e76ce2 100644 >> --- a/pinyin-isearch-loaders.el >> +++ b/pinyin-isearch-loaders.el >> @@ -76,10 +76,10 @@ Argument QUAIL-FILE \"quail/PY.el\" for example." >> >> (defun pinyin-isearch-loaders--punct-quail-filter (rules) >> "Load RULES for single letters of punctuations." >> - (seq-filter (lambda (x) (= (length (car x)) 1)) rules)) >> + (seq-filter (lambda (x) (length= (car x))) rules)) >> >> >> -;; ---------- load quail/PY.el for chinese hierogliphs --------- >> +;; ---------- load quail/PY.el for chinese hieroglyphs --------- >> >> (defun pinyin-isearch-loaders--py-rules-loader () >> "Load quail rules and add lv and nv to lu and nu. >> diff --git a/pinyin-isearch-pinyin.el b/pinyin-isearch-pinyin.el >> index ea8c122..98ef538 100644 >> --- a/pinyin-isearch-pinyin.el >> +++ b/pinyin-isearch-pinyin.el >> @@ -27,10 +27,9 @@ >> >> ;;; Commentary: >> >> - >> ;; Allow to search with Chinese pinyin diacritical tone marks for >> -;; pinyin with diacritical tone marks with or without abolity to >> -;; fallback to normal latin text. >> +;; pinyin with diacritical tone marks with or without abolity to >> +;; fallback to normal latin text. >> >> ;; Features: >> ;; - white spaces are ignored between syllables, >> @@ -92,7 +91,7 @@ >> ;; ("v" "[ūúǔùǖǘǚǜ]") >> ("ue" "ü[ēéěè]") >> ;; ("ve" "ü[ēéěè]") >> -)) >> + )) >> >> (defconst pinyin-isearch-pinyin-vowel-table-normal >> '(("a" "[aāáǎà]") >> @@ -102,7 +101,7 @@ >> ("u" "[uūúǔùǖǘǚǜ]") >> ("ue" "[uü][eēéěè]") >> ;; ("ve" "[uü][eēéěè]") >> -)) >> + )) >> >> (defconst pinyin-isearch-pinyin-vowels >> '(("ā" "a") >> @@ -112,7 +111,7 @@ >> ("ū" "u") >> ("ǖ" "u") >> ("üē" "ue")) >> - "Used to convert sisheng pinyin to toneless pinyin.") >> + "Used to convert sisheng pinyin to toneless pinyin.") >> >> >> (defun pinyin-isearch-pinyin--sisheng-to-normal (syllable) >> @@ -132,7 +131,7 @@ Requires `sisheng-regexp' and `sisheng-vowel-table' to be defined." >> (let* ((matched-vowel (downcase (match-string 0 syllable))) >> ;; Lookup normalized vowel in table >> (vowel-list (cdr (assoc-string matched-vowel sisheng-vowel-table))) >> - (base-vowel (when vowel-list (nth 0 vowel-list)))) >> + (base-vowel (and vowel-list (nth 0 vowel-list)))) >> ;; Special handling for 'v' and 've' which represent 'ü' and 'üe' >> (cond >> ((equal base-vowel "v") (setq base-vowel "u")) >> @@ -156,13 +155,17 @@ Requires `sisheng-regexp' and `sisheng-vowel-table' to be defined." >> sisheng-syllable-table)))) >> >> >> + >> +;; why are you using underscores in the function name? >> (defun pinyin-isearch-pinyin--get_vowel_from_sisheng (string) >> - "For input \"zuō\" get \"o\". >> + "For input \"zuō\" get \"o\". >> Uses: constant `pinyin-isearch-pinyin-vowels'. >> Argument STRING sisheng syllable." >> (string-match sisheng-regexp string) >> (let* ((vowel-match (downcase (match-string 0 string))) >> (vowel-list >> + ;; `assoc-string' supports case-insensitive matching, you >> + ;; don't need the `downcase' call before. >> (cdr (assoc-string vowel-match pinyin-isearch-pinyin-vowels)))) >> (car vowel-list))) >> >> @@ -189,7 +192,8 @@ and global variable `pinyin-isearch-pinyin-syllable-table'." >> (if syl >> (let ((new-vow (pinyin-isearch-pinyin--get_vowel_from_sisheng syl))) >> (if (not (member new-vow vowels)) >> - (setq vowels (cons (pinyin-isearch-pinyin--get_vowel_from_sisheng syl) vowels)) >> + (push (pinyin-isearch-pinyin--get_vowel_from_sisheng syl) >> + vowels) >> ;; else ignore previous long syllable with same vowel >> (setq ret nil)))) >> ;; save position of the first longest syllable >> @@ -221,18 +225,17 @@ Argument VOWELS list of normal vowels as a result of function >> pinyin-isearch-pinyin-vowel-table-normal >> ;; else >> pinyin-isearch-pinyin-vowel-table))) >> - (if (= (length vowels) 1) >> - (car (cdr (assoc-string (car vowels) vowel-table))) >> + (if (length= vowels 1) >> + (cadr (assoc-string (car vowels) vowel-table)) >> ;; else lenght > 1 >> (let* ((pin-vowels (mapcar (lambda (x) >> - (assoc-string x vowel-table)) vowels)) >> + (assoc-string x vowel-table)) >> + vowels)) >> (pin-vowels1 (car (cdr (car pin-vowels)))) >> (pin-vowels2 (car (cdr (car (cdr pin-vowels))))) >> (second-vowel (car (cdr vowels))) >> (second-vowel-regex >> - (if (or (string-prefix-p "a" second-vowel t) >> - (string-prefix-p "o" second-vowel t) >> - (string-prefix-p "e" second-vowel t)) >> + (if (string-match-p "\\`[AEOaeo]" second-vowel) >> (concat "['’]?" pin-vowels2) ;; inject this before second vowel >> ;; else >> (concat "\\s-*" second-vowel)))) >> @@ -298,7 +301,7 @@ Uses functions: >> (let* ((st (regexp-quote string)) >> (len (length st)) >> (first-syllable-stat (when (> len 1) >> - (pinyin-isearch-pinyin--get-position-first-syllable st))) >> + (pinyin-isearch-pinyin--get-position-first-syllable st))) >> (first-syllable-pos (car first-syllable-stat))) >> (if first-syllable-pos >> (let* ((first-syllable (substring string 0 first-syllable-pos)) >> @@ -312,7 +315,7 @@ Uses functions: >> (if (or (not pinyin-isearch-full-fallback) >> pinyin-isearch-strict >> (string-empty-p string)) >> - "$^" >> + regexp-unmatchable >> ;; else >> st)))) >> >> diff --git a/pinyin-isearch.el b/pinyin-isearch.el >> index 25a92e7..146ee9d 100644 >> --- a/pinyin-isearch.el >> +++ b/pinyin-isearch.el >> @@ -1,6 +1,6 @@ >> ;;; pinyin-isearch.el --- Pinyin mode for isearch -*- lexical-binding: t -*- >> >> -;; Copyright (c) 2024 Anoncheg >> +;; Copyright (c) 2024-2026 Anoncheg >> >> ;; Maintainer: Anoncheg <[email protected]> >> ;; Author: Anoncheg <[email protected]> >> @@ -70,7 +70,7 @@ >> ;; Search variants: >> >> ;; By default active search for both pinyin and chinese characters >> -;; with fallback to latin normal characters. >> +;; with fallback to latin normal characters. >> >> ;;;; Other packages: >> >> @@ -96,12 +96,12 @@ >> ;; - ('’) in (Fāng'àn) apostrophe, syllable delimiter (隔音符号), >> ;; is a strict grammatical requirement used to prevent ambiguity. >> ;; Apostrophe use is rare, generally at word boundaries or within >> -;; compounds. to prevent misreading when a syllable starting >> +;; compounds. to prevent misreading when a syllable starting >> ;; with a, e, or o follows another syllable directly. >> ;; - Upperacase for pinyin. >> ;; - Cangjie search >> ;; - method for getting pinyin for chinese characters. >> - ;; - allow connecting other input methods. >> +;; - allow connecting other input methods. >> >> ;;; Code: >> >> @@ -132,8 +132,7 @@ For pinyin search we enable search only if first syllable is real and >> By default we are looking for all characters and pinyin syllables that >> start with typed by user first part of syllable." >> :local t >> - :type 'boolean >> - :group 'pinyin-isearch) >> + :type 'boolean) >> >> (defcustom pinyin-isearch-full-fallback t >> "Non-nil means search for normal normal latin letters also. >> @@ -144,8 +143,7 @@ For pinyin search this means we use latin symbols in regex for vowels to >> syllable was not found pinyin search dont look for latin string. >> Used in `pinyin-isearch-chars--concat-variants'." >> :local t >> - :type 'boolean >> - :group 'pinyin-isearch) >> + :type 'boolean) >> >> (defcustom pinyin-isearch-default-mode 'both >> "Select target text in `pinyin-isearch-mode'. >> @@ -159,8 +157,7 @@ both of them. Used for mode `pinyin-isearch-mode', and functions >> (const :tag "Search in both: pinyin and Chinese characters strictly" strict-both) >> (const :tag "Search in Chinese characters strictly, same as t" strict-characters) >> (const :tag "Search in pinyin only, same as nil" pinyin) >> - (const :tag "Search in pinyin only, same as pinyin" nil)) >> - :group 'pinyin-isearch) >> + (const :tag "Search in pinyin only, same as pinyin" nil))) >> >> (defcustom pinyin-isearch-fix-jumping-flag t >> "Non-nil means fix isearch behavior. >> @@ -169,8 +166,7 @@ success found occurance, not from when you begin whole search. >> This fix force isearch to begin from the starting point. >> Disable for native isearch behavior." >> :local t >> - :type 'boolean >> - :group 'pinyin-isearch) >> + :type 'boolean) >> >> (defvar-local pinyin-isearch--original-search-default-mode search-default-mode >> "Used in `pinyin-isearch--set-isearch' to save previous state.") >> @@ -208,7 +204,7 @@ Argument STRING is a query string, LAX is not used." >> "Help subfunction to replace isearch functions. >> Used in functions `pinyin-isearch-forward' and >> `pinyin-isearch-backward'." >> - (cond >> + (cond ;pcase would be nice here >> ;; both >> ((eq pinyin-isearch-default-mode 'both) >> #'pinyin-isearch-both-regexp-function) >> @@ -317,7 +313,7 @@ Optional argument NO-RECURSIVE-EDIT see original function `isearch-backward'." >> ;; This ensures standard options (like M-s _ for symbol search) still fall back correctly >> (set-keymap-parent map (lookup-key isearch-mode-map (kbd "M-s"))) >> map) >> - "Keymap for \\M -s bindings inside `pinyin-isearch-mode'.") >> + "Keymap for \\[search-map] bindings inside `pinyin-isearch-mode'.") >> >> >> ;;;###autoload >> @@ -334,7 +330,7 @@ normal search. >> characters isearch submode. >> - M -s n to activate default pinying-isearch mode. >> - M -s r to activate standard search." >> - :lighter " p-isearch" :global nil :group 'isearch >> + :lighter " p-isearch" :global nil :group 'isearch ;why are you adding this to isearch, and not your group? >> :keymap (let ((map (make-sparse-keymap))) >> (define-key map (kbd "C-s") #'pinyin-isearch-forward) >> (define-key map (kbd "C-r") #'pinyin-isearch-backward) >> >> I am not sure how to say this, but the code seems somewhat chaotic >> (which is also a reason I am interested in the LLM question above). I >> see that a lot is going on in many functions, but neither the name, the >> docstring or the actual code gives me a good idea of what exactly is >> happening. Perhaps this is just domain specific knowledge, but add the >> many stray spaces all over the code, I am wondering if it wouldn't make >> sense for you to wait a bit and polish the code before adding it to >> ELPA? This is just a suggestion, not a final call. >> >>> This package is on MELPA for over a year https://stable.melpa.org/#/pinyin-isearch >>> >>> Package allow to search with pinyin without diacritical marks in >>> pinyin text and chinese characters. Accurate regex for search created >>> that match all variants. >>> >>> Implemented as Emacs Isearch modification. >>> >>> For example: to find "Shànghǎi" and "上海" in text you just type: C-s shanghai. >>> >>> Animated screenshot: https://codeberg.org/Anoncheg/public-share/raw/branch/main/pinyin-isearch.gif >>> >>> Feel free to reach out for anything. >>> >>> From 0a107ac4360fb8c1c528b0e50c2a55a3cac024d0 Mon Sep 17 00:00:00 2001 >>> From: Vitaliy Chepelev <[email protected]> >>> Date: Thu, 9 Jul 2026 16:23:02 +0000 >>> Subject: [PATCH] Add pinyin-isearch to NonGNU ELPA >>> >>> --- >>> elpa-packages | 4 ++++ >>> 1 file changed, 4 insertions(+) >>> >>> diff --git a/elpa-packages b/elpa-packages >>> index aa670d7..4bea7ce 100644 >>> --- a/elpa-packages >>> +++ b/elpa-packages >>> @@ -790,6 +790,10 @@ >>> :news "CHANGELOG.md" >>> :ignored-files ("LICENSE")) >>> >>> + (pinyin-isearch :url "https://codeberg.org/Anoncheg/pinyin-isearch" >>> + :readme "README.md" >> >> Are you sure this is a good source for a package description? The file >> starts with images, links to README_zh.md and instructions on how to >> install the package. Also, in the line starting with "2) Install via" >> you seem to mix Markdown and Org notation for inline code. >> >>> + :ignored-files (".github" "pinyin-isearch-chars-tests.el" "pinyin-isearch-pinyin-tests.el" "pinyin-isearch-main-tests.el" "tests.sh" "Makefile")) >> >> You don't need this, since you already have an .elpaignore file. >> >>> + >>> (popon :url "https://codeberg.org/akib/emacs-popon") >>> >>> (popup :url "https://github.com/auto-complete/popup-el" >>> -- >>> 2.54.0 > > -- > Best regards, > . > Python+Java+Bash+Lisp Data Scientist, AI Engineer > . > PGP pub key: DB8A27FEDA7B4545603396FFC81251BC87659B81 -- Best regards, . Python+Java+Bash+Lisp Data Scientist, AI Engineer . PGP pub key: DB8A27FEDA7B4545603396FFC81251BC87659B81