From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mp2 ([2001:41d0:2:4a6f::]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits)) by ms11 with LMTPS id COeyNZeNu19RNgAA0tVLHw (envelope-from ) for ; Mon, 23 Nov 2020 10:23:19 +0000 Received: from aspmx1.migadu.com ([2001:41d0:2:4a6f::]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits)) by mp2 with LMTPS id qGqOMZeNu19IGQAAB5/wlQ (envelope-from ) for ; Mon, 23 Nov 2020 10:23:19 +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 347E0940630 for ; Mon, 23 Nov 2020 10:23:16 +0000 (UTC) Received: from localhost ([::1]:47132 helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1kh906-0008Ds-8F for larch@yhetil.org; Mon, 23 Nov 2020 05:23:14 -0500 Received: from eggs.gnu.org ([2001:470:142:3::10]:37198) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1kh8zd-0008DS-H7 for emacs-orgmode@gnu.org; Mon, 23 Nov 2020 05:22:45 -0500 Received: from grinta.net ([109.74.203.128]:50558) by eggs.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1kh8za-0006FE-SU for emacs-orgmode@gnu.org; Mon, 23 Nov 2020 05:22:45 -0500 Received: from black.local (ip-109-41-67-118.web.vodafone.de [109.41.67.118]) (Authenticated sender: daniele) by grinta.net (Postfix) with ESMTPSA id 1004AEDD11; Mon, 23 Nov 2020 10:22:32 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=simple/simple; d=grinta.net; s=2020; t=1606126958; bh=diNp7XJF9jBAVsRzi1C0Gqx6JAf9NFPFK6Wxagd/7ms=; h=Subject:To:Cc:References:From:Date:In-Reply-To:From; b=OZzOeAZlt2Rw7JpYskj+ifODJUOgmSM5TtDNO3kYAv8cU1ReRQoR5WRLplqmN9rwx uyR01OvNAmDWhNf8hLkPRTOTMxSb6KzxKzSG2aQPlHF5dpZiYXSr6AWEhx86U4c/eP rn3Zr/G7SI2ZG1jOfW9LHa9YHaP1KG/mSNQQfDTMG6393T84Y+r/GKehS4uKUVG+P5 VRoPBLh9Wi75BhWtnz8IzGc6GC5kRn5Wk3Tac6jhJGboxv8lBLUpJ8paL5Nxx5B3vs pYKXWANHGTtaG4BSfOitSzCaZKbqyAkWnOpyWIvTPW6TiCkpIe/QgjJfBrFrpZ5rMt 6VOk2fOkVVQVw== Subject: Re: [PATCH] org-table: Add mode flag to enable Calc units simplification mode To: Kyle Meyer References: <48c15b01-341d-f4c6-7086-1a39e4977868@grinta.net> <87h7pgvk6b.fsf@kyleam.com> From: Daniele Nicolodi Message-ID: <938fa4a5-f162-6c03-072b-4f11546a95c8@grinta.net> Date: Mon, 23 Nov 2020 11:22:24 +0100 User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.14; rv:78.0) Gecko/20100101 Thunderbird/78.5.0 MIME-Version: 1.0 In-Reply-To: <87h7pgvk6b.fsf@kyleam.com> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Received-SPF: pass client-ip=109.74.203.128; envelope-from=daniele@grinta.net; helo=grinta.net X-Spam_score_int: 12 X-Spam_score: 1.2 X-Spam_bar: + X-Spam_report: (1.2 / 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, NICE_REPLY_A=-0.001, RCVD_IN_SBL_CSS=3.335, SPF_HELO_PASS=-0.001, SPF_PASS=-0.001 autolearn=no 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=fail (headers rsa verify failed) header.d=grinta.net header.s=2020 header.b=OZzOeAZl; 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: -0.01 X-TUID: xTJnJVDoik0o 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