Hello Kyle,
thank you for the review. It is much appreciated as lisp (and Emacs lisp
in particular) is not the language I am most fluent in.
On 23/11/2020 04:14, Kyle Meyer wrote:
> Daniele Nicolodi writes:
>
>> Subject: [PATCH 1/4] org-table: Fix table formula mode string handling
>>
>> * lisp/org-table.el (org-table-eval-formula): Move mode lookup table
>> from org-table--set-calc-mode to here.
>>
>> * lisp/org-table.el (org-table--set-calc-mode): Use plist-put instead
>> of the buggy open coded version.
>
> So I think the "buggy" is referring to your analysis in
> <6d8c15c2-d1b0-d913-df39-c60381cff...@grinta.net>:
>
> The first (if ...) does some value substitutions, however, IIUC the
> second (if ...) sets a new value for an entry in the org-tbl-calc-modes
> plist if the entry is already present and builds a new plist with the
> entry prepended if the entry is not there. However, the original plist
> is returned and not the one with the new entry prepended.
>
> And...
>
>> ---
>> lisp/org-table.el | 24 ++++++++++--------------
>> 1 file changed, 10 insertions(+), 14 deletions(-)
>>
>> diff --git a/lisp/org-table.el b/lisp/org-table.el
>> index 112b1e171..0790dc3ca 100644
>> --- a/lisp/org-table.el
>> +++ b/lisp/org-table.el
>> @@ -721,17 +721,8 @@ Field is restored even in case of abnormal exit."
>> (org-table-goto-column ,column)
>> (set-marker ,line nil)))))
>>
>> -(defsubst org-table--set-calc-mode (var &optional value)
>> - (if (stringp var)
>> - (setq var (assoc var '(("D" calc-angle-mode deg)
>> - ("R" calc-angle-mode rad)
>> - ("F" calc-prefer-frac t)
>> - ("S" calc-symbolic-mode t)))
>> - value (nth 2 var) var (nth 1 var)))
>> - (if (memq var org-tbl-calc-modes)
>> - (setcar (cdr (memq var org-tbl-calc-modes)) value)
>> - (cons var (cons value org-tbl-calc-modes)))
>> - org-tbl-calc-modes)
>> +(defsubst org-table--set-calc-mode (var value)
>> + (plist-put org-tbl-calc-modes var value))
>
> ...that does look to be the case. Do you have an example that triggers
> the issue? If so, it'd be good to cover that in test-org-table.el.
calc-simplify-mode is not part of the default calc mode plist, thus
adding it to the plist does not work without this change.
> However, looking ahead, org-table--set-calc-mode is dropped in the last
> patch. I'd suggest instead dropping org-table--set-calc-mode and moving
> to using cl-getf as part of this first patch. (I know that'd require a
> bit of reworking since it touches changes from patches 2 and 3.)
I can do this.
>> Subject: [PATCH 2/4] org-table: Simplify mode string parsing
>>
> [...]
>> ;;; Predicates
>> @@ -2427,54 +2427,42 @@ location of point."
>> (org-tbl-calc-modes (copy-sequence org-calc-default-modes))
>> (numbers nil) ; was a variable, now fixed default
>> (keep-empty nil)
>> - n form form0 formrpl formrg bw fmt x ev orig c lispp literal
>> + form form0 formrpl formrg bw fmt ev orig lispp literal
>> duration duration-output-format)
>> ;; Parse the format string. Since we have a lot of modes, this is
>> ;; a lot of work. However, I think calc still uses most of the time.
>> - (if (string-match ";" formula)
>> - (let ((tmp (org-split-string formula ";")))
>> - (setq formula (car tmp)
>> - fmt (concat (cdr (assoc "%" org-table-local-parameters))
>> - (nth 1 tmp)))
>> + (if (string-match "\\(.*\\);\\(.*\\)" formula)
>> + (progn
>> + (setq fmt (concat (match-string-no-properties 2 formula)
>> + (cdr (assoc "%" org-table-local-parameters)))
>> + formula (match-string-no-properties 1 formula))
>
> This patch's changes look good to me. As a minor comment, I'd prefer if
> the rewritten parts (here and for the entire series) only used one pair
> per setq call, even if it's not worth updating the entire function to
> use this style.
Funny. This is also the style I prefer, but I wrote the code to conform
to the style used in this context. I can fix this too.
>> Subject: [PATCH 3/4] org-table: Add mode flag to enable Calc units
>> simplification mode
>>
>> * org-table.el (org-table-eval-formula): Add the `u` mode flag to
>> enable Calc's units simplification mode.
>
> Neat. As far as I can tell, this works nicely.
>
>> ---
>> lisp/org-table.el | 3 ++-
>> 1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/lisp/org-table.el b/lisp/org-table.el
>> index 4baad2600..6b92656bd 100644
>> --- a/lisp/org-table.el
>> +++ b/lisp/org-table.el
>> @@ -2447,11 +2447,12 @@ location of point."
>> (?e (org-table--set-calc-mode 'calc-float-format (list 'eng
>> n)))))
>> ;; Remove matched flags from the mode string.
>> (setq fmt (replace-match "" t t fmt)))
>> - (while (string-match "\\([tTUNLEDRFS]\\)" fmt)
>> + (while (string-match "\\([tuTUNLEDRFS]\\)" fmt)
>> (let ((c (string-to-char (match-string 1 fmt))))
>> (cl-case c
>> (?t (setq duration t numbers t
>> duration-output-format
>> org-table-duration-custom-format))
>> + (?u (org-table--set-calc-mode 'calc-simplify-mode 'units))
>> (?T (setq duration t numbers t duration-output-format nil))
>> (?U (setq duration t numbers t duration-output-format 'hh:mm))
>> (?N (setq numbers t))
>
> A nit-pick about ordering: I think it'd be better to not nestle "u" in
> between "t" and "T" because it invites the reader to incorrectly assume
> that "u" is somehow connected to "t", "T", and "U".
>
> You already mentioned that you plan to add documentation. It'd also be
> good to add a test to test-org-table.el and a NEWS entry.
I thought alphabetical ordering was the most natural. Which other
ordering would make sense?
>> Subject: [PATCH 4/4] org-table: Remove unused org-tbl-calc-modes variable
>>
>> * org-table.el (org-tbl-calc-modes): Remove the variable declaration
>> as the varialble is only used as a local variable in
>> `org-table-eval-formula'.
>>
>> * org-table.el (org-table--set-calc-mode): Drop convenience macro.
>>
>> * org-table.el (org-table-eval-formula): Rename `org-tbl-calc-modes`
>> local variable without the org-table prefix and usr the gained screen
>> real estate to avoid indirection through covenience macro.
>
> s/usr/use/
>
> Sounds good to me. As I mentioned, I'd like to see this absorbed into
> the first patch of the series.
I'll send an updated series later today.
Cheers,
Dan
