emacs-orgmode@gnu.org archives
 help / color / mirror / code / Atom feed
From: Kyle Meyer <kyle@kyleam.com>
To: Daniele Nicolodi <daniele@grinta.net>
Cc: Org Mode List <emacs-orgmode@gnu.org>
Subject: Re: [PATCH] org-table: Add mode flag to enable Calc units simplification mode
Date: Sun, 22 Nov 2020 22:14:36 -0500	[thread overview]
Message-ID: <87h7pgvk6b.fsf@kyleam.com> (raw)
In-Reply-To: <48c15b01-341d-f4c6-7086-1a39e4977868@grinta.net>

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.


  parent reply	other threads:[~2020-11-23  3:15 UTC|newest]

Thread overview: 25+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-10-24 15:33 Daniele Nicolodi
2020-11-07 14:03 ` Daniele Nicolodi
2020-11-19  5:58   ` Kyle Meyer
2020-11-19 20:02     ` Daniele Nicolodi
2020-11-23  3:14 ` Kyle Meyer [this message]
2020-11-23 10:22   ` Daniele Nicolodi
2020-11-23 22:27     ` Kyle Meyer
2020-11-24  0:03       ` Daniele Nicolodi
2020-11-24  5:35         ` Kyle Meyer
2020-11-24  8:05           ` Daniele Nicolodi
2020-11-25  2:07             ` Kyle Meyer
2020-11-25  7:41               ` Christian Moe
2020-11-23 10:25   ` Daniele Nicolodi
2020-11-23 22:25     ` Kyle Meyer
2020-11-23 23:01       ` Daniele Nicolodi
  -- strict thread matches above, loose matches on Subject: below --
2020-10-19 15:38 Bug in org-table--set-calc-mode? Daniele Nicolodi
2020-10-20 13:30 ` [PATCH] org-table: Add mode flag to enable Calc units simplification mode Daniele Nicolodi
2020-10-20 13:44   ` Eric S Fraga
2020-10-20 14:00     ` Daniele Nicolodi
2020-10-20 14:22       ` Eric S Fraga
2020-10-20 14:19     ` Eric S Fraga
2020-10-20 14:32       ` Daniele Nicolodi
2020-10-20 14:53         ` Daniele Nicolodi
2020-10-20 15:35           ` Eric S Fraga
2020-10-20 15:35         ` Eric S Fraga
2020-10-21 15:57   ` Daniele Nicolodi

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: https://www.orgmode.org/

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=87h7pgvk6b.fsf@kyleam.com \
    --to=kyle@kyleam.com \
    --cc=daniele@grinta.net \
    --cc=emacs-orgmode@gnu.org \
    --subject='Re: [PATCH] org-table: Add mode flag to enable Calc units simplification mode' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Code repositories for project(s) associated with this inbox:

	https://git.savannah.gnu.org/cgit/emacs/org-mode.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).