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

[Daniele Nicolodi <daniele@grinta.net>]

[-- Attachment #1: Type: text/plain, Size: 3159 bytes --]

Thank you for the review, Kyle.

Another updated patch set is attached.

Cheers,
Dan


On 24/11/2020 06:35, Kyle Meyer wrote:
> Daniele Nicolodi writes:
> 
>> Subject: [PATCH 1/3] org-table: Remove unused org-tbl-calc-modes variable
>>  declaration
> 
> Looks good.
> 
>> Subject: [PATCH 2/3] org-table: Simplify mode string parsing
>> and reduce scope of local variables.
> [...]
>> -      (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))))
> 
> Hmm, the concat arguments are getting swapped.  Intentional?
> 
> The rest looks good.
> 
>> Subject: [PATCH 3/3] 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.
>>
>> * test-org-table.el (test-org-table/mode-string-u): Add Unit test for
>> the new mode flag.
>>
>> * org-manual.org: Document new mode flag.
>> ---
>>  doc/org-manual.org             |  8 ++++++++
>>  etc/ORG-NEWS                   |  7 ++++++-
>>  lisp/org-table.el              |  5 +++--
>>  testing/lisp/test-org-table.el | 12 ++++++++++++
>>  4 files changed, 29 insertions(+), 3 deletions(-)
> 
> Thanks for the additions.
> 
>> diff --git a/doc/org-manual.org b/doc/org-manual.org
>> index be69996d5..70b748fc7 100644
>> --- a/doc/org-manual.org
>> +++ b/doc/org-manual.org
>> @@ -2075,6 +2075,14 @@ variable ~org-calc-default-modes~.
>>  
>>    Fraction and symbolic modes of Calc.
>>  
>> +- =u= ::
>> +
>> +  Units simplification mode of Calc. Calc is also a symbolic
> 
> convention nit: two spaces after a period
> 
>> +  calculator and is capable of working with values having an unit
>> +  (numerals followed by an unit string in Org table cells). This mode
>> +  instructs Calc to simplify the units in the computed expression
>> +  before returning the result.
>> +
>>  - =T=, =t=, =U= ::
>>  
>>    Duration computations in Calc or Lisp, [[*Durations and time values]].
>> diff --git a/etc/ORG-NEWS b/etc/ORG-NEWS
>> index 889eb4aab..6f6db8e43 100644
>> --- a/etc/ORG-NEWS
>> +++ b/etc/ORG-NEWS
>> @@ -60,7 +60,7 @@ relative links within a project as follows:
>>  #+end_src
>>  
>>  ** New features
>> -*** =ob-python= improvements to =:return= header argument 
>> +*** =ob-python= improvements to =:return= header argument
> 
> unrelated space change
> 
>>  The =:return= header argument in =ob-python= now works for session
>>  blocks as well as non-session blocks.  Also, it now works with the
>> @@ -112,6 +112,11 @@ package, to convert pandas Dataframes into orgmode tables:
>>  | 2 | 3 | 6 |
>>  #+end_src
>>  
>> +*** New =u= table formula flag to enable Calc units simplification mode
>> +
>> +A new =u= mode flat for Calc formulas in Org tables has been added to
>> +enable Calc units simplification mode.
> 
> s/flat/flag/
> 


[-- Attachment #2: 0001-org-table-Remove-unused-org-tbl-calc-modes-variable-.patch --]
[-- Type: text/plain, Size: 4395 bytes --]

From d2ab4d06e19620c0347425861e4534cde8656543 Mon Sep 17 00:00:00 2001
From: Daniele Nicolodi <daniele@grinta.net>
Date: Mon, 23 Nov 2020 23:44:51 +0100
Subject: [PATCH 1/3] org-table: Remove unused org-tbl-calc-modes variable
 declaration

* org-table.el (org-tbl-calc-modes): Remove variable declaration as
the varialble is used only within `org-table-eval-formula'.

* org-table.el (org-table-eval-formula): Rename `org-tbl-calc-modes`
local variable without the `org-tbl-` prefix and use the gained screen
real estate to avoid indirection through covenience macro. This
requires moving the mode lookup table from `org-table--set-calc-mode`
to here.

* org-table.el (org-table--set-calc-mode): Drop convenience macro.
Note that the macro was not working as intended when the caller tried
to add a new entry in the plist as in this case the macro would create
a new plist with the added entry but return the old one.
---
 lisp/org-table.el | 42 +++++++++++++++---------------------------
 1 file changed, 15 insertions(+), 27 deletions(-)

diff --git a/lisp/org-table.el b/lisp/org-table.el
index 112b1e171..a3b73a828 100644
--- a/lisp/org-table.el
+++ b/lisp/org-table.el
@@ -676,8 +676,6 @@ Will be filled automatically during use.")
     ("_" . "Names for values in row below this one.")
     ("^" . "Names for values in row above this one.")))
 
-(defvar org-tbl-calc-modes nil)
-
 (defvar org-pos nil)
 
 \f
@@ -721,18 +719,6 @@ 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)
-
 \f
 ;;; Predicates
 
@@ -2433,7 +2419,7 @@ location of point."
 			equation
 		      (org-table-get-formula equation (equal arg '(4)))))
 	   (n0 (org-table-current-column))
-	   (org-tbl-calc-modes (copy-sequence org-calc-default-modes))
+	   (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
@@ -2449,14 +2435,12 @@ location of point."
 	      (setq c (string-to-char (match-string 1 fmt))
 		    n (string-to-number (match-string 2 fmt)))
 	      (if (= c ?p)
-		  (setq org-tbl-calc-modes
-			(org-table--set-calc-mode 'calc-internal-prec n))
-		(setq org-tbl-calc-modes
-		      (org-table--set-calc-mode
-		       'calc-float-format
-		       (list (cdr (assoc c '((?n . float) (?f . fix)
-					     (?s . sci) (?e . eng))))
-			     n))))
+		  (setf (cl-getf calc-modes 'calc-internal-prec) n)
+		(setf (cl-getf calc-modes
+			       'calc-float-format)
+			       (list (cdr (assoc c '((?n . float) (?f . fix)
+						     (?s . sci) (?e . eng))))
+				     n)))
 	      (setq fmt (replace-match "" t t fmt)))
 	    (if (string-match "[tTU]" fmt)
 		(let ((ff (match-string 0 fmt)))
@@ -2476,9 +2460,13 @@ location of point."
 		(setq keep-empty t
 		      fmt (replace-match "" t t fmt)))
 	    (while (string-match "[DRFS]" fmt)
-	      (setq org-tbl-calc-modes
-		    (org-table--set-calc-mode (match-string 0 fmt)))
-	      (setq fmt (replace-match "" t t fmt)))
+	      (let* ((c (string-to-char (match-string 0 fmt)))
+		     (mode (cdr (assoc c '((?D calc-angle-mode deg)
+					   (?R calc-angle-mode rad)
+					   (?F calc-prefer-frac t)
+					   (?S calc-symbolic-mode t))))))
+		(setf (cl-getf calc-modes (car mode)) (cadr mode))
+		(setq fmt (replace-match "" t t fmt))))
 	    (unless (string-match "\\S-" fmt)
 	      (setq fmt nil))))
       (when (and (not suppress-const) org-table-formula-use-constants)
@@ -2621,7 +2609,7 @@ location of point."
 
 	  (setq ev (if (and duration (string-match "^[0-9]+:[0-9]+\\(?::[0-9]+\\)?$" form))
 		       form
-		     (calc-eval (cons form org-tbl-calc-modes)
+		     (calc-eval (cons form calc-modes)
 				(when (and (not keep-empty) numbers) 'num)))
 		ev (if duration (org-table-time-seconds-to-string
 				 (if (string-match "^[0-9]+:[0-9]+\\(?::[0-9]+\\)?$" ev)
-- 
2.29.2


[-- Attachment #3: 0002-org-table-Simplify-mode-string-parsing.patch --]
[-- Type: text/plain, Size: 5481 bytes --]

From 1c2f246e438483826906e62e17d4312724d1190d Mon Sep 17 00:00:00 2001
From: Daniele Nicolodi <daniele@grinta.net>
Date: Tue, 24 Nov 2020 00:19:09 +0100
Subject: [PATCH 2/3] org-table: Simplify mode string parsing

* org-table.el (org-table-eval-formula): Simplify mode string parsing
and reduce scope of local variables.
---
 lisp/org-table.el | 93 +++++++++++++++++++++--------------------------
 1 file changed, 42 insertions(+), 51 deletions(-)

diff --git a/lisp/org-table.el b/lisp/org-table.el
index a3b73a828..cf1bfa31c 100644
--- a/lisp/org-table.el
+++ b/lisp/org-table.el
@@ -2422,51 +2422,42 @@ location of point."
 	   (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 (cdr (assoc "%" org-table-local-parameters))
+			      (match-string-no-properties 2 formula)))
+	    (setq formula (match-string-no-properties 1 formula))
 	    (while (string-match "\\([pnfse]\\)\\(-?[0-9]+\\)" fmt)
-	      (setq c (string-to-char (match-string 1 fmt))
-		    n (string-to-number (match-string 2 fmt)))
-	      (if (= c ?p)
-		  (setf (cl-getf calc-modes 'calc-internal-prec) n)
-		(setf (cl-getf calc-modes
-			       'calc-float-format)
-			       (list (cdr (assoc c '((?n . float) (?f . fix)
-						     (?s . sci) (?e . eng))))
-				     n)))
+	      (let ((c (string-to-char (match-string 1 fmt)))
+		    (n (string-to-number (match-string 2 fmt))))
+		(cl-case c
+		  (?p (setf (cl-getf calc-modes 'calc-internal-prec) n))
+		  (?n (setf (cl-getf calc-modes 'calc-float-format) (list 'float n)))
+		  (?f (setf (cl-getf calc-modes 'calc-float-format) (list 'fix n)))
+		  (?s (setf (cl-getf calc-modes 'calc-float-format) (list 'sci n)))
+		  (?e (setf (cl-getf calc-modes '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)
+	      (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))
+		  (?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))
+		  (?L (setq literal t))
+		  (?E (setq keep-empty t))
+		  (?D (setf (cl-getf calc-modes 'calc-angle-mode) 'deg))
+		  (?R (setf (cl-getf calc-modes 'calc-angle-mode) 'rad))
+		  (?F (setf (cl-getf calc-modes 'calc-prefer-frac) t))
+		  (?S (setf (cl-getf calc-modes 'calc-symbolic-mode) t))))
+	      ;; Remove matched flags from the mode string.
 	      (setq fmt (replace-match "" t t fmt)))
-	    (if (string-match "[tTU]" fmt)
-		(let ((ff (match-string 0 fmt)))
-		  (setq duration t numbers t
-			duration-output-format
-			(cond ((equal ff "T") nil)
-			      ((equal ff "t") org-table-duration-custom-format)
-			      ((equal ff "U") 'hh:mm))
-			fmt (replace-match "" t t fmt))))
-	    (if (string-match "N" fmt)
-		(setq numbers t
-		      fmt (replace-match "" t t fmt)))
-	    (if (string-match "L" fmt)
-		(setq literal t
-		      fmt (replace-match "" t t fmt)))
-	    (if (string-match "E" fmt)
-		(setq keep-empty t
-		      fmt (replace-match "" t t fmt)))
-	    (while (string-match "[DRFS]" fmt)
-	      (let* ((c (string-to-char (match-string 0 fmt)))
-		     (mode (cdr (assoc c '((?D calc-angle-mode deg)
-					   (?R calc-angle-mode rad)
-					   (?F calc-prefer-frac t)
-					   (?S calc-symbolic-mode t))))))
-		(setf (cl-getf calc-modes (car mode)) (cadr mode))
-		(setq fmt (replace-match "" t t fmt))))
 	    (unless (string-match "\\S-" fmt)
 	      (setq fmt nil))))
       (when (and (not suppress-const) org-table-formula-use-constants)
@@ -2567,17 +2558,17 @@ location of point."
 	(setq form0 form)
 	;; Insert the references to fields in same row
 	(while (string-match "\\$\\(\\([-+]\\)?[0-9]+\\)" form)
-	  (setq n (+ (string-to-number (match-string 1 form))
-		     (if (match-end 2) n0 0))
-		x (nth (1- (if (= n 0) n0 (max n 1))) fields)
-		formrpl (save-match-data
-			  (org-table-make-reference
-			   x keep-empty numbers lispp)))
-	  (when (or (not x)
-		    (save-match-data
-		      (string-match (regexp-quote formula) formrpl)))
-	    (user-error "Invalid field specifier \"%s\""
-			(match-string 0 form)))
+	  (let* ((n (+ (string-to-number (match-string 1 form))
+		       (if (match-end 2) n0 0)))
+		 (x (nth (1- (if (= n 0) n0 (max n 1))) fields)))
+	    (setq formrpl (save-match-data
+			    (org-table-make-reference
+			     x keep-empty numbers lispp)))
+	    (when (or (not x)
+		      (save-match-data
+			(string-match (regexp-quote formula) formrpl)))
+	      (user-error "Invalid field specifier \"%s\""
+			  (match-string 0 form))))
 	  (setq form (replace-match formrpl t t form)))
 
 	(if lispp
-- 
2.29.2


[-- Attachment #4: 0003-org-table-Add-mode-flag-to-enable-Calc-units-simplif.patch --]
[-- Type: text/plain, Size: 3673 bytes --]

From 2842d6f0a10ee83d8d500cd3e4c3827a32418eef Mon Sep 17 00:00:00 2001
From: Daniele Nicolodi <daniele@grinta.net>
Date: Tue, 24 Nov 2020 00:49:16 +0100
Subject: [PATCH 3/3] 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.

* test-org-table.el (test-org-table/mode-string-u): Add Unit test for
the new mode flag.

* org-manual.org: Document new mode flag.
---
 doc/org-manual.org             |  8 ++++++++
 etc/ORG-NEWS                   |  5 +++++
 lisp/org-table.el              |  5 +++--
 testing/lisp/test-org-table.el | 12 ++++++++++++
 4 files changed, 28 insertions(+), 2 deletions(-)

diff --git a/doc/org-manual.org b/doc/org-manual.org
index be69996d5..d9de5e633 100644
--- a/doc/org-manual.org
+++ b/doc/org-manual.org
@@ -2075,6 +2075,14 @@ variable ~org-calc-default-modes~.
 
   Fraction and symbolic modes of Calc.
 
+- =u= ::
+
+  Units simplification mode of Calc.  Calc is also a symbolic
+  calculator and is capable of working with values having an unit,
+  represented with numerals followed by an unit string in Org table
+  cells.  This mode instructs Calc to simplify the units in the
+  computed expression before returning the result.
+
 - =T=, =t=, =U= ::
 
   Duration computations in Calc or Lisp, [[*Durations and time values]].
diff --git a/etc/ORG-NEWS b/etc/ORG-NEWS
index 889eb4aab..0d08a939c 100644
--- a/etc/ORG-NEWS
+++ b/etc/ORG-NEWS
@@ -112,6 +112,11 @@ package, to convert pandas Dataframes into orgmode tables:
 | 2 | 3 | 6 |
 #+end_src
 
+*** New =u= table formula flag to enable Calc units simplification mode
+
+A new =u= mode flag for Calc formulas in Org tables has been added to
+enable Calc units simplification mode.
+
 ** Miscellaneous
 *** =org-goto-first-child= now works before first heading
 
diff --git a/lisp/org-table.el b/lisp/org-table.el
index cf1bfa31c..c138190ef 100644
--- a/lisp/org-table.el
+++ b/lisp/org-table.el
@@ -2442,7 +2442,7 @@ location of point."
 		  (?e (setf (cl-getf calc-modes '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 "\\([tTUNLEDRFSu]\\)" fmt)
 	      (let ((c (string-to-char (match-string 1 fmt))))
 		(cl-case c
 		  (?t (setq duration t numbers t
@@ -2455,7 +2455,8 @@ location of point."
 		  (?D (setf (cl-getf calc-modes 'calc-angle-mode) 'deg))
 		  (?R (setf (cl-getf calc-modes 'calc-angle-mode) 'rad))
 		  (?F (setf (cl-getf calc-modes 'calc-prefer-frac) t))
-		  (?S (setf (cl-getf calc-modes 'calc-symbolic-mode) t))))
+		  (?S (setf (cl-getf calc-modes 'calc-symbolic-mode) t))
+		  (?u (setf (cl-getf calc-modes 'calc-simplify-mode) 'units))))
 	      ;; Remove matched flags from the mode string.
 	      (setq fmt (replace-match "" t t fmt)))
 	    (unless (string-match "\\S-" fmt)
diff --git a/testing/lisp/test-org-table.el b/testing/lisp/test-org-table.el
index fb9d83f95..1c930c8d0 100644
--- a/testing/lisp/test-org-table.el
+++ b/testing/lisp/test-org-table.el
@@ -380,6 +380,18 @@ reference (with row).  Mode string N."
 "
      1 calc)))
 
+(ert-deftest test-org-table/mode-string-u ()
+  "Basic: verify that mode string u results in units
+simplification mode applied to Calc formulas."
+  (org-test-table-target-expect
+   "
+| 1.5 A/B | 2.0 B | |
+"
+   "
+| 1.5 A/B | 2.0 B | 3. A |
+"
+   1 "#+TBLFM: $3=$1*$2;u"))
+
 (ert-deftest test-org-table/lisp-return-value ()
   "Basic: Return value of Lisp formulas."
   (org-test-table-target-expect
-- 
2.29.2