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 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.
next prev 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://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 \ /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