Org-mode mailing list
 help / color / mirror / Atom feed
From: Daniele Nicolodi <daniele@grinta.net>
To: Kyle Meyer <kyle@kyleam.com>
Cc: Org Mode List <emacs-orgmode@gnu.org>
Subject: Re: [PATCH] org-table: Add mode flag to enable Calc units simplification mode
Date: Mon, 23 Nov 2020 11:22:24 +0100
Message-ID: <938fa4a5-f162-6c03-072b-4f11546a95c8@grinta.net> (raw)
In-Reply-To: <87h7pgvk6b.fsf@kyleam.com>

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

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


  reply	other threads:[~2020-11-23 10:23 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
2020-11-23 10:22   ` Daniele Nicolodi [this message]
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://orgmode.org

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

  git send-email \
    --in-reply-to=938fa4a5-f162-6c03-072b-4f11546a95c8@grinta.net \
    --to=daniele@grinta.net \
    --cc=emacs-orgmode@gnu.org \
    --cc=kyle@kyleam.com \
    /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

Org-mode mailing list

This inbox may be cloned and mirrored by anyone:

	git clone --mirror https://orgmode.org/list/0 list/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 list list/ https://orgmode.org/list \
		emacs-orgmode@gnu.org
	public-inbox-index list

Example config snippet for mirrors.
Newsgroups are available over NNTP:
	nntp://news.yhetil.org/yhetil.emacs.orgmode
	nntp://news.gmane.io/gmane.emacs.orgmode


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git