From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mp0 ([2001:41d0:2:4a6f::]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits)) by ms11 with LMTPS id CDCOIkspu1/RNQAA0tVLHw (envelope-from ) for ; Mon, 23 Nov 2020 03:15:23 +0000 Received: from aspmx1.migadu.com ([2001:41d0:2:4a6f::]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits)) by mp0 with LMTPS id oK/YHUspu19cfgAA1q6Kng (envelope-from ) for ; Mon, 23 Nov 2020 03:15:23 +0000 Received: from lists.gnu.org (lists.gnu.org [209.51.188.17]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by aspmx1.migadu.com (Postfix) with ESMTPS id C44C0940482 for ; Mon, 23 Nov 2020 03:15:22 +0000 (UTC) Received: from localhost ([::1]:54816 helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1kh2K0-0007sz-0Q for larch@yhetil.org; Sun, 22 Nov 2020 22:15:20 -0500 Received: from eggs.gnu.org ([2001:470:142:3::10]:45634) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1kh2JS-0007sg-RI for emacs-orgmode@gnu.org; Sun, 22 Nov 2020 22:14:46 -0500 Received: from pb-smtp20.pobox.com ([173.228.157.52]:58187) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1kh2JQ-00052B-9i for emacs-orgmode@gnu.org; Sun, 22 Nov 2020 22:14:46 -0500 Received: from pb-smtp20.pobox.com (unknown [127.0.0.1]) by pb-smtp20.pobox.com (Postfix) with ESMTP id CC0AF102E71; Sun, 22 Nov 2020 22:14:40 -0500 (EST) (envelope-from kyle@kyleam.com) DKIM-Signature: v=1; a=rsa-sha1; c=relaxed; d=pobox.com; h=from:to:cc :subject:in-reply-to:references:date:message-id:mime-version :content-type; s=sasl; bh=zDHh6Nb5Q4rLfEY4Ez90oJ6caJM=; b=yiAYaw zYjrqjwtrAIMbJJxbRjO/GWi834F1+cgOIjpxlUga4BXKKzSljHhs/5LeLfVi2m0 EbYH+6O1ZyrSKyZhXLcTks8Ld3b/S0AQgYELSsQjCbr24GEG/t34ryeaDtI+N0bI Ff1lqGJceB0rwWb1upncYFHe4v6bHx89lOut4= Received: from pb-smtp20.sea.icgroup.com (unknown [127.0.0.1]) by pb-smtp20.pobox.com (Postfix) with ESMTP id C4C74102E70; Sun, 22 Nov 2020 22:14:40 -0500 (EST) (envelope-from kyle@kyleam.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed; d=kyleam.com; h=from:to:cc:subject:in-reply-to:references:date:message-id:mime-version:content-type; s=mesmtp; bh=aAChKd0PdtdezPGWVi7tNvs52l4xdyqxGT59FdB1UaU=; b=tWDccOvMya5jH4UpLKmbb4L6gxL1fV7srnnhCxGP/X/oODLNxbS10KrfCfnAmOM+EcEuL7jHL/eNGBG4easLEAZYyyjxYujX6eNRf1s2RBPhtJOWTBzBfB5htnxb0CpvgXvI+NF18L4TJji9ll7Vjdk8CR7QUWBGIYsTW+dFYBA= Received: from localhost (unknown [45.33.91.115]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by pb-smtp20.pobox.com (Postfix) with ESMTPSA id 22380102E6F; Sun, 22 Nov 2020 22:14:38 -0500 (EST) (envelope-from kyle@kyleam.com) From: Kyle Meyer To: Daniele Nicolodi Subject: Re: [PATCH] org-table: Add mode flag to enable Calc units simplification mode In-Reply-To: <48c15b01-341d-f4c6-7086-1a39e4977868@grinta.net> References: <48c15b01-341d-f4c6-7086-1a39e4977868@grinta.net> Date: Sun, 22 Nov 2020 22:14:36 -0500 Message-ID: <87h7pgvk6b.fsf@kyleam.com> MIME-Version: 1.0 Content-Type: text/plain X-Pobox-Relay-ID: 05156E78-2D3A-11EB-87C6-E43E2BB96649-24757444!pb-smtp20.pobox.com Received-SPF: pass client-ip=173.228.157.52; envelope-from=kyle@kyleam.com; helo=pb-smtp20.pobox.com X-Spam_score_int: -27 X-Spam_score: -2.8 X-Spam_bar: -- X-Spam_report: (-2.8 / 5.0 requ) BAYES_00=-1.9, DKIM_SIGNED=0.1, DKIM_VALID=-0.1, DKIM_VALID_AU=-0.1, DKIM_VALID_EF=-0.1, RCVD_IN_DNSWL_LOW=-0.7, SPF_HELO_NONE=0.001, SPF_PASS=-0.001 autolearn=ham autolearn_force=no X-Spam_action: no action X-BeenThere: emacs-orgmode@gnu.org X-Mailman-Version: 2.1.23 Precedence: list List-Id: "General discussions about Org-mode." List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: Org Mode List Errors-To: emacs-orgmode-bounces+larch=yhetil.org@gnu.org Sender: "Emacs-orgmode" X-Scanner: ns3122888.ip-94-23-21.eu Authentication-Results: aspmx1.migadu.com; dkim=pass header.d=pobox.com header.s=sasl header.b=yiAYaw z; dkim=pass header.d=kyleam.com header.s=mesmtp header.b=tWDccOvM; dmarc=none; spf=pass (aspmx1.migadu.com: domain of emacs-orgmode-bounces@gnu.org designates 209.51.188.17 as permitted sender) smtp.mailfrom=emacs-orgmode-bounces@gnu.org X-Spam-Score: -1.21 X-TUID: tXI4XLAAiSx+ 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.