[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [NonGNU ELPA] New package: latex-table-wizard
From: |
Philip Kaludercic |
Subject: |
Re: [NonGNU ELPA] New package: latex-table-wizard |
Date: |
Fri, 16 Dec 2022 21:37:23 +0000 |
Enrico Flor <enrico@eflor.net> writes:
> Thank you so much for your comments! I implemented your many
> suggestions wrt. the code. I must say I didn't use to have all the
> backquotes but then I read somewhere that you should prefer
>
> `(,x ,y)
>
> over
>
> (list x y)
>
> and so I replaced all the instances of the latter with the former. I
> probably misunderstood the "advice".
I cannot think of why, after all
(macroexpand-all '`(,a ,b ,c)) => (list a b c)
and I prefer the latter, as it just seems cleaner. I'd be curious to
hear the original argument, because to me that sounds like using a
feature for it's own sake.
> I also added the .elpaignore, removed the .info file and added a short
> description.txt file to serve as readme.
That might work as well, but I think you might also use the ";;;
Commentary" section in the main file for the same purpose.
> From c711b1b314668ab7eacf7bab3d9f38471380ab5f Mon Sep 17 00:00:00 2001
> From: Enrico Flor <nericoflor@gmail.com>
> Date: Fri, 16 Dec 2022 16:16:44 -0500
> Subject: [PATCH] Add latex-table-wizard
>
> ---
> elpa-packages | 9 +++++++--
> 1 file changed, 7 insertions(+), 2 deletions(-)
>
> diff --git a/elpa-packages b/elpa-packages
> index 8254411cb2..b2ddceca10 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,11 @@
> (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"
> + :readme "description.txt"
> + :news "NEWS"
> + :doc "latex-table-wizard.texi")
> +
> (lorem-ipsum :url "https://github.com/jschaf/emacs-lorem-ipsum"
> :readme "README.md")
>
> --
> 2.38.1
>
>
>
> "Philip Kaludercic" <philipk@posteo.net> writes:
>
>> 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
Re: [NonGNU ELPA] New package: latex-table-wizard, Philip Kaludercic, 2022/12/17