Re: [PATCH] org-table: Add mode flag to enable Calc units simplification mode

[Kyle Meyer <kyle@kyleam.com>]

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-c60381cff70b@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.

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.)

> 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.

> 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.

> 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.

Thanks again.