Re: [NonGNU ELPA] New package: latex-table-wizard

[Philip Kaludercic]

Enrico Flor <enrico@eflor.net> writes:

> I would like to submit latex-table-wizard to NonGNU ELPA.  This package
> depends on AucTeX and on transient, and provides a minor mode with a
> series of commands to navigate and edit complicated LaTeX table-like
> environments (the standard ones, but the user can define additional
> ones).
>
> With a transient UI, this package allows you to:
>
> + navigate "logically" (that is, move by cells)
>
> + insert or kill rows or column
>
> + move arbitrary cells or groups of cells around
>
> + align the table in different ways (however alignment is not needed for
>   the functionalities above)
>
> These commands are not fooled by the presence of embedded tables or
> other complications (for example: while editing a larger table, a buffer
> substring like:
>
>     & ... \makecell{ a & b \\ c & d} ... &
>
> is still parsed as a single cell).
>
> From 27f25c72ed8e0e3e81cfc4f996f8c03c9c0155fe Mon Sep 17 00:00:00 2001
> From: Enrico Flor <nericoflor@gmail.com>
> Date: Fri, 16 Dec 2022 10:58:55 -0500
> Subject: [PATCH] Add latex-table-wizard
>
> ---
>  elpa-packages | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/elpa-packages b/elpa-packages
> index 8254411cb2..90356989cb 100644
> --- a/elpa-packages
> +++ b/elpa-packages
> @@ -78,7 +78,7 @@
>    )
>
>   (cdlatex            :url "https://github.com/cdominik/cdlatex";)
> -
> +
>   (cider                      :url "https://github.com/clojure-emacs/cider";
>    :ignored-files ("LICENSE" "doc" "logo" "refcard" "test")
>    :news "CHANGELOG.md")
> @@ -117,7 +117,7 @@
>    :news "changelog.rst")
>
>   (dockerfile-mode    :url "https://github.com/spotify/dockerfile-mode";)
> -
> +
>   (dracula-theme              :url "https://github.com/dracula/emacs";
>    :ignored-files ("INSTALL.md" "screenshot.png" "start_emacs_test.sh" 
> "test-profile.el"))
>
> @@ -434,6 +434,12 @@
>   (kotlin-mode                :url 
> "https://github.com/Emacs-Kotlin-Mode-Maintainers/kotlin-mode";
>    :ignored-files ("doc" "test" "Cask" "Makefile"))
>
> + (latex-table-wizard    :url 
> "https://github.com/enricoflor/latex-table-wizard";

Here are a few comments in diff-form, just from reading the code:

diff --git a/latex-table-wizard.el b/latex-table-wizard.el
index 0238ae3..0b487af 100644
--- a/latex-table-wizard.el
+++ b/latex-table-wizard.el
@@ -90,7 +90,7 @@
 
 (require 'latex)
 (require 'seq)
-(require 'rx)
+(eval-when-compile (require 'rx))
 (require 'regexp-opt)
 (eval-when-compile (require 'subr-x))
 (require 'transient)
@@ -121,13 +121,11 @@ Capture group 1 matches the name of the macro.")
 
 (defcustom latex-table-wizard-column-delimiters '("&")
   "List of strings that are column delimiters if unescaped."
-  :type '(repeat string)
-  :group 'latex-table-wizard)
+  :type '(repeat string))
 
 (defcustom latex-table-wizard-row-delimiters '("\\\\\\\\")
   "List of strings that are row delimiters if unescaped."
-  :type '(repeat string)
-  :group 'latex-table-wizard)
+  :type '(repeat string))
 
 (defcustom latex-table-wizard-hline-macros '("cline"
                                              "vline"
@@ -139,8 +137,7 @@ Capture group 1 matches the name of the macro.")
 
 Each member of this list is a string that would be between the
 \"\\\" and the arguments."
-  :type '(repeat string)
-  :group 'latex-table-wizard)
+  :type '(repeat string))
 
 (defcustom latex-table-wizard-new-environments-alist nil
   "Alist mapping environment names to property lists.
@@ -167,10 +164,9 @@ of a macro that inserts some horizontal line.  For a macro
   :type '(alist :key-type (string :tag "Name of the environment:")
                 :value-type (plist :key-type symbol
                                    :options (:col :row :lines)
-                                   :value-type (repeat string)))
-
-  :group 'latex-table-wizard)
+                                   :value-type (repeat string))))
 
+;; Why not use `memq'?
 (defmacro latex-table-wizard--or (symbol &rest values)
   "Return non-nil if SYMBOL is `eq' to one of VALUES."
   (let ((bools (mapcar (lambda (value) `(eq ,symbol ,value))
@@ -452,18 +448,15 @@ is a cons cell of the form (P . V), where P is either
 If prop-val2 is nil, it is assumed that TABLE is a list of cells
 that only differ for the property in the car of PROP-VAL1 (in
 other words, that TABLE is either a column or a row)"
-  (if prop-val2
-      (catch 'cell
+  (catch 'cell
+    (if prop-val2
         (dolist (x table)
           (when (and (= (cdr prop-val1) (plist-get x (car prop-val1)))
                      (= (cdr prop-val2) (plist-get x (car prop-val2))))
             (throw 'cell x)))
-        nil)
-    (catch 'cell
       (dolist (x table)
         (when (= (cdr prop-val1) (plist-get x (car prop-val1)))
-          (throw 'cell x)))
-      nil)))
+          (throw 'cell x))))))
 
 (defun latex-table-wizard--sort (table same-line dir)
   "Return a sorted table, column or row given TABLE.
@@ -492,10 +485,10 @@ F, C precedes D and so on; and if DIR is either 
\\='next\\=' or
          (copy-table (copy-sequence table)))
     (if (not same-line)
         (sort copy-table (lambda (x y)
-                           (let ((rows `(,(plist-get x :row)
-                                         ,(plist-get y :row)))
-                                 (cols `(,(plist-get x :column)
-                                         ,(plist-get y :column))))
+                           (let ((rows (list (plist-get x :row)
+                                             (plist-get y :row)))
+                                 (cols (list (plist-get x :column)
+                                             (plist-get y :column))))
                              (cond ((and vert (apply #'= cols))
                                     (apply #'< rows))
                                    (vert
@@ -536,10 +529,9 @@ beginning of the available portion of the buffer."
         (catch 'found
           (while (re-search-forward regexp nil t)
             (when (<= (match-beginning group) position (match-end group))
-              (throw 'found `(,(match-string-no-properties group)
-                              ,(match-beginning group)
-                              ,(match-end group))))
-            nil))))))
+              (throw 'found (list (match-string-no-properties group)
+                                  (match-beginning group)
+                                  (match-end group))))))))))
 
 
 
@@ -732,6 +724,9 @@ If SAME-LINE is non-nil, never leave current column or row."
 X and Y are each a list of the form \\='(B E)\\=', where B and E
 are markers corresponding to the beginning and the end of the
 buffer substring."
+  ;; Instead of assuming the form of X and Y, wouldn't it be better to
+  ;; destruct these and make sure?  Then you could also avoid using
+  ;; `apply'?
   (save-excursion
     (let ((x-string (concat " "
                             (string-trim
@@ -821,7 +816,7 @@ Don't print any message if NO-MESSAGE is non-nil."
              (message "Cell (%s,%s) selected for swapping"
                       (plist-get sel :column)
                       (plist-get sel :row)))
-           (latex-table-wizard--hl-cells `(,sel)))
+           (latex-table-wizard--hl-cells (list sel)))
           ((eq thing 'row)
            (unless no-message
              (message "Row %s selected for swapping"
@@ -1261,44 +1256,10 @@ information about how transient suffixes are defined 
(that is,
 how the data stored in this variable and in
 `latex-table-wizard-default-keys' contributes to the definition
 of the transient prefix)."
-  :type '(alist :key-type
-                (symbol :tag "Command:"
-                        :options (toggle-truncate-lines
-                                  exchange-point-and-mark
-                                  universal-argument
-                                  transient-quit-all
-                                  undo
-                                  latex-table-wizard-right
-                                  latex-table-wizard-right
-                                  latex-table-wizard-left
-                                  latex-table-wizard-up
-                                  latex-table-wizard-down
-                                  latex-table-wizard-end-of-row
-                                  latex-table-wizard-beginning-of-row
-                                  latex-table-wizard-top
-                                  latex-table-wizard-bottom
-                                  latex-table-wizard-beginning-of-cell
-                                  latex-table-wizard-end-of-cell
-                                  latex-table-wizard-mark-cell
-                                  latex-table-wizard-swap-cell-right
-                                  latex-table-wizard-swap-cell-left
-                                  latex-table-wizard-swap-cell-up
-                                  latex-table-wizard-swap-cell-down
-                                  latex-table-wizard-swap-column-right
-                                  latex-table-wizard-swap-column-left
-                                  latex-table-wizard-swap-row-up
-                                  latex-table-wizard-swap-row-down
-                                  latex-table-wizard-align
-                                  latex-table-wizard-select-column
-                                  latex-table-wizard-select-row
-                                  latex-table-wizard-select-deselect-cell
-                                  latex-table-wizard-deselect-all
-                                  latex-table-wizard-swap
-                                  latex-table-wizard-insert-column
-                                  latex-table-wizard-insert-row
-                                  latex-table-wizard-kill-column
-                                  latex-table-wizard-kill-row))
-                :value-type string)
+  :type `(alist :key-type
+                (symbol :tag "Command:")
+                :value-type string
+                :options ,(mapcar #'car latex-table-wizard-default-keys))
   :group 'latex-table-wizard)
 
 (defun latex-table-wizard--make-suffix (symbol)

> +  :readme "readme.org"

Are you sure you want to use your "readme.org" file as the package
"readme"?  The contents will be displayed when a user uses
describe-package to find out more about what latex-table-wizard does,
and in my experience it is better to keep it short instead of presenting
a wall of text, that is usually better kept part of the manual.

> +  :doc "latex-table-wizard.texi"

It also appears you have a .info file in your repository that isn't
needed.  That will be generated by the ELPA server, so you don't need to
track the "binary" file in your repository.

If you _do_ intent to build the manual yourself, you should mention the
.info file here, not the .texi file.

> +  :news "NEWS"
> +  :ignored-files ("COPYING"))

Instead of listing files in the package specification, it would be
better to maintain a .elpaignore file in your own repository.

>   (lorem-ipsum           :url "https://github.com/jschaf/emacs-lorem-ipsum";
>    :readme "README.md")
>
> --
> 2.38.1