Re: [ELPA] some tex-related packages

[Philip Kaludercic]

Paul Nelson <ultrono@gmail.com> writes:

> Hi Philip,
>
>> I've taken a look at one package, and am adding the comments to this
>> message, and hope to do the same for the others, step by step:
>
> I suspect your backlog remains much the same as a few months ago, but
> figured I'd send a friendly nudge to see if you might have the chance
> to review any further packages from the list, i.e., one of
>
> https://github.com/ultronozm/tex-parens.el

Here are my comments/questions for this package:

diff --git a/tex-parens.el b/tex-parens.el
index fb93734..a2b4c5d 100644
--- a/tex-parens.el
+++ b/tex-parens.el
@@ -40,8 +40,10 @@
 
 ;;; Code:
 
+(eval-when-compile (require 'rx))
+
 (defgroup tex-parens ()
-  "Like lisp.el but for tex."
+  "Like lisp.el but for tex."          ;I'd try to elaborate what this means
   :group 'tex)
 
 (defun tex-parens--beginning-of-defun ()
@@ -64,7 +66,7 @@ Here `defun' means top-level environment."
     ("\\lfloor" . "\\rfloor")
     ("\\lceil" . "\\rceil"))
   "Left/right pairs of delimiters."
-  :type '(repeat (cons string string)))
+  :type '(repeat (cons (string :tag "Left pair") (string :tag "Right pair"))))
 
 (defcustom tex-parens-solo-delimiters
   '("|" "\\|" "\\vert" "\\Vert")
@@ -98,11 +100,11 @@ Here `defun' means top-level environment."
   "Maximum length of a delimiter.
 This is comfortably larger than `\\biggl\\langle' and
 `\\begin{proposition}', for example."
-  :type 'integer)
+  :type 'natnum)
 
 (defun tex-parens--reduce-append (func list1 list2)
   "List consisting of FUNC applied to pairs from LIST1 and LIST2."
-  (seq-reduce #'append
+  (seq-reduce #'append                 ;couldn't you use `mapcan'?
               (mapcar (lambda (item1)
                         (mapcar (lambda (item2)
                                   (funcall func item1 item2))
@@ -166,19 +168,19 @@ form delimiters which are visibly `left'/`opening' or
   ;; If AUCTeX 14.0.5+ is installed, then we make some of the
   ;; navigation commands automatically open previews and folds.
   (when (boundp 'preview-auto-reveal-commands)
-    (dolist (func '(tex-parens-down-list
-                    tex-parens-backward-down-list))
+    (dolist (func (list #'tex-parens-down-list
+                       #'tex-parens-backward-down-list))
       (add-to-list 'preview-auto-reveal-commands func)))
 
   (when (boundp 'TeX-fold-auto-reveal-commands)
-    (dolist (func '(tex-parens-down-list
-                    tex-parens-backward-down-list
-                    tex-parens-up-list
-                    tex-parens-backward-up-list
-                    tex-parens-forward-list
-                    tex-parens-backward-list
-                    tex-parens-forward-sexp
-                    tex-parens-backward-sexp))
+    (dolist (func (list #'tex-parens-down-list
+                       #'tex-parens-backward-down-list
+                       #'tex-parens-up-list
+                       #'tex-parens-backward-up-list
+                       #'tex-parens-forward-list
+                       #'tex-parens-backward-list
+                       #'tex-parens-forward-sexp
+                       #'tex-parens-backward-sexp))
       (add-to-list 'TeX-fold-auto-reveal-commands func)))
 
   (setq-local tex-parens--saved-beginning-of-defun-function
@@ -190,8 +192,8 @@ form delimiters which are visibly `left'/`opening' or
   (setq tex-parens--pairs (tex-parens--generate-pairs))
   (setq tex-parens--pairs-swap
         (mapcar (lambda (x) (cons (cdr x) (car x))) tex-parens--pairs))
-  (setq tex-parens--delims (append (mapcar #'car tex-parens--pairs)
-                                   (mapcar #'cdr tex-parens--pairs)))
+  (setq tex-parens--delims (nconc (mapcar #'car tex-parens--pairs)
+                                  (mapcar #'cdr tex-parens--pairs)))
 
   (setq tex-parens--regexp
         (concat
@@ -244,23 +246,22 @@ Tex Parens mode is a minor mode in which 
lisp/sexp/defun-based commands
 are adapted to tex environments and math delimiters.  The affected
 commands include, for instance, `forward-sexp', `forward-list' and
 `beginning-of-defun'."
-  :lighter nil
-  :keymap (let ((map (make-sparse-keymap)))
+  :keymap (let ((map (make-sparse-keymap))) ;why not define it in a
+                                           ;separate variable? it
+                                           ;would also avoid overlong
+                                           ;lines.
             (define-key map [remap forward-sexp] #'tex-parens-forward-sexp)
             (define-key map [remap backward-sexp] #'tex-parens-backward-sexp)
             (define-key map [remap forward-list] #'tex-parens-forward-list)
             (define-key map [remap backward-list] #'tex-parens-backward-list)
-            (define-key map [remap backward-up-list]
-                        #'tex-parens-backward-up-list)
+            (define-key map [remap backward-up-list] 
#'tex-parens-backward-up-list)
             (define-key map [remap up-list] #'tex-parens-up-list)
             (define-key map [remap down-list] #'tex-parens-down-list)
             (define-key map [remap delete-pair] #'tex-parens-delete-pair)
             (define-key map [remap mark-sexp] #'tex-parens-mark-sexp)
             (define-key map [remap kill-sexp] #'tex-parens-kill-sexp)
-            (define-key map [remap backward-kill-sexp]
-                        #'tex-parens-backward-kill-sexp)
-            (define-key map [remap transpose-sexps]
-                        #'tex-parens-transpose-sexps)
+            (define-key map [remap backward-kill-sexp] 
#'tex-parens-backward-kill-sexp)
+            (define-key map [remap transpose-sexps] 
#'tex-parens-transpose-sexps)
             (define-key map [remap raise-sexp] #'tex-parens-raise-sexp)
             map)
   (cond
@@ -401,7 +402,7 @@ delimiter.  Otherwise, return nil."
    (and
     (stringp delim)
     (or
-     (and (or
+     (and (or                          ;I'd use `rx' here
            (string-match "\\\\begin{\\([^}]+\\)}\\[[^]]+\\]" delim)
            (string-match "\\\\begin{\\([^}]+\\)}" delim))
           (let ((type (match-string 1 delim)))
@@ -506,6 +507,7 @@ Search up to BOUND."
       (when tex-parens--debug
         (message "Unmatched delimiters: %s" (car stack))))))
 
+;; doesn't this function share a lot of code with the previous function?
 (defun tex-parens--backward-list-1 (&optional bound)
   "Move backward across one balanced group.
 Search up to BOUND."
@@ -1096,38 +1098,43 @@ This regexp matches the start of various math 
environments, keeping a
 couple of characters before the primary match to leave space for Avy."
   :type 'regexp)
 
+(declare-function avy-jump "avy" (regex &rest _))
 (defun tex-parens-avy-jump-to-math ()
   "Jump inside a math expression using Avy.
 This function uses `tex-parens-avy-regexp' to identify potential math
 expressions, then jumps to the selected one and moves point inside the
 expression."
   (interactive)
-  (if (fboundp 'avy-jump)
-      (avy-jump tex-parens-avy-regexp
-                :action (lambda (pos)
-                          (goto-char pos)
-                          (forward-char 2)
-                          (tex-parens-down-list)
-                          ;; For preview-auto-reveal:
-                          (setq this-command #'tex-parens-down-list)))
-    (user-error "Avy is not available.  Please install and load it to use this 
function")))
+  (unless (and (fboundp 'avy-jump)
+              (require 'avy nil t)
+              (fboundp 'avy-jump))
+    (error "Avy is not available.  Please install it to use this function"))
+  (avy-jump tex-parens-avy-regexp
+            :action (lambda (pos)
+                      (goto-char pos)
+                      (forward-char 2)
+                      (tex-parens-down-list)
+                      ;; For preview-auto-reveal:
+                      (setq this-command #'tex-parens-down-list))))
 
 (defun tex-parens-avy-copy-math ()
   "Copy a math expression selected using Avy.
 This function uses `tex-parens-avy-regexp' to identify potential math
 expressions, then copies the selected one to the kill ring."
   (interactive)
-  (if (fboundp 'avy-jump)
-      (avy-jump tex-parens-avy-regexp
-                :action (lambda (pos)
-                          (let ((beg (+ pos 2))
-                                (end (save-excursion
-                                       (goto-char (+ pos 2))
-                                       (tex-parens-forward-list)
-                                       (point))))
-                            (copy-region-as-kill beg end)
-                            (message "Math expression copied"))))
-    (user-error "Avy is not available.  Please install and load it to use this 
function")))
+  (unless (and (fboundp 'avy-jump)
+              (require 'avy nil t)
+              (fboundp 'avy-jump))
+    (error "Avy is not available.  Please install it to use this function"))
+  (avy-jump tex-parens-avy-regexp
+            :action (lambda (pos)
+                      (let ((beg (+ pos 2))
+                            (end (save-excursion
+                                   (goto-char (+ pos 2))
+                                   (tex-parens-forward-list)
+                                   (point))))
+                        (copy-region-as-kill beg end)
+                        (message "Math expression copied")))))
 
 (provide 'tex-parens)
 ;;; tex-parens.el ends here

> https://github.com/ultronozm/auctex-label-numbers.el

No special comments here, perhaps check if things like user option types
could be made more specific.

> https://github.com/ultronozm/preview-auto.el

Just a few comments here:

diff --git a/preview-auto.el b/preview-auto.el
index 7614b4f..da184a2 100644
--- a/preview-auto.el
+++ b/preview-auto.el
@@ -39,6 +39,7 @@
 
 ;;; Code:
 
+(eval-when-compile (require 'cl-lib))
 (require 'latex)
 (require 'preview)
 
@@ -90,7 +91,6 @@ Should work in AUCTeX `LaTeX-mode' buffers.  Implemented using
                  (memq math-face face))))
     (texmathp)))
 
-
 (defun preview-auto--generate-rules ()
   "Return list of rules for identifying math environments."
   (let* ((basic-rules
@@ -120,7 +120,6 @@ is valid.")
 (defvar-local preview-auto--begin-re nil
   "Regular expression for identifying the beginning of a math environment.")
 
-
 (defun preview-auto--check-default ()
   "Default predicate for checking whether to consider a delimiter.
 Returns non-nil if either
@@ -543,6 +542,7 @@ cancel the preview, so that the preview is not misplaced."
     (remove-hook 'after-change-functions #'preview-auto--after-change t)
     (remove-hook 'post-command-hook #'preview-auto--post-command t))))
 
+;;;###autoload
 (defun preview-auto-setup ()
   "Hook function for installing bind and menu item."
   (remove-hook 'LaTeX-mode-hook #'preview-auto-setup)
@@ -552,6 +552,9 @@ cancel the preview, so that the preview is not misplaced."
    ["automatically" preview-auto-mode]
    "(or toggle) at point"))
 
+;; Please don't modify hooks on the top-level!  Even if it removes
+;; itself, it is better to advise the user to call this or by hand or
+;; copy/adjust the code as they see fit.
 (add-hook 'LaTeX-mode-hook #'preview-auto-setup)
 
 (provide 'preview-auto)

> https://github.com/ultronozm/tex-item.el

Here nothing noteworthy to say from my side.

> https://github.com/ultronozm/preview-tailor.el

Again, nothing special to say.  I just think you should include cl-lib
and seq (or just one of the two).

> (I recall tex-parens being raised as something that might be usefully
> included in Emacs, so maybe that would be a natural next target?)
>
> Thanks, best,
>
> Paul

-- 
        Philip Kaludercic on siskin