Re: [PATCH] org-plot abstractions and extension
TEC writes: > Kyle Meyer writes: > >> recent org-plot example from 8d5122fc5: >> [...] >> That could be rewritten as [...] > > Would you like me to bundle that change in somewhere? In my view it's fine to leave it as is (it's just an example that was fresh in my mind), but of course feel free to send a patch for it if you'd like. > One question, I saw Bastien say that we didn't want whitespace-only > commits, so how should whitespace-fixups be done? I think the idea is to avoid whitespace-only churn, particularly if the only goal is tab/space indentation consistency. In that case, you'd just leave it be unless you're changing the code for some other reason. On the other hand, I think a dedicated patch for the indentation fix you had in the first commit would be welcomed. The distinction is that in this case the code is simply misaligned: (mapcar (lambda (row) (org--plot/values-stats (mapcar #'string-to-number (cdr row)) hard-min hard-max)) table) I'd say that's worth fixing because it hurts readability. Also, in the process, it'd be good to move "table" to the next line because (to use lisp-mode's phrasing) it's "hidden behind a deeper element". > Subject: [PATCH] org-plot.el: fix compiler warnings Thanks. Applied (7a9a8a56a)... > > * lisp/org-plot.el (org--plot/values-stats): Replace `log10' with > `log'. > (org--plot/nice-frequency-pick): Replace obsolete `case' with `pcase`. ... with s/pcase/cl-case/ to match the final state of the code.
Re: [PATCH] org-plot abstractions and extension
Kyle Meyer writes:
> Regardless of what you tend to use, you used "case" here in 73c99bf42;
> the minimal fix is to add a cl- prefix, and any other switch with the
> justification that "case is obsolete" is likely to raise a reviewer's
> eyebrow.
This makes sense.
> cl-case isn't in cl-lib, and there is no need to load anything.
Huh, interesting.
> recent org-plot example from 8d5122fc5:
> [...]
> That could be rewritten as [...]
Would you like me to bundle that change in somewhere?
@@ -210,9 +210,9 @@ values, namely regarding the range."
"From a the values in a TABLE of data, attempt to guess an appropriate
number of ticks."
(let* ((row-data
(mapcar (lambda (row) (org--plot/values-stats
- (mapcar #'string-to-number (cdr row))
- hard-min
- hard-max)) table))
+ (mapcar #'string-to-number (cdr row))
+ hard-min
+ hard-max)) table))
>>>
>>> Please drop this unrelated space change.
>>
>> Erm, this isn't unrelated. As the function being called changed length,
>> the indentation of the arguments is thus also changed.
>
> This change is in org--plot/sensible-tick-num. I don't spot any
> non-whitespace changes there. Git appears to agree with me:
>
> $ git show | grep '@@ -210,9'
> @@ -210,9 +210,9 @@ (defun org--plot/sensible-tick-num (table
> hard-min hard-max)
> $ git show -w | grep '@@ -210,9'
Ooops, I thought you were referring to one of the other regions (I saw
the "let" and "mapcar" and my brain pattern-matched the rest :P)
One question, I saw Bastien say that we didn't want whitespace-only
commits, so how should whitespace-fixups be done?
>> Subject: [PATCH] org-plot.el: fix compiler warnings
>>
>> * (org--plot/values-stats): Replace `log10' with `log'.
>
> Please add a file name ("lisp/org-plot.el") to the start of the
> changelog entry.
Ah, forgot I needed that. Sorted :)
(final?) patch revision attached.
--
Timothy
>From c4c7b835f27b65111859d030af58a8317a82b0a0 Mon Sep 17 00:00:00 2001
From: TEC
Date: Thu, 24 Dec 2020 02:26:17 +0800
Subject: [PATCH] org-plot.el: fix compiler warnings
* lisp/org-plot.el (org--plot/values-stats): Replace `log10' with
`log'.
(org--plot/nice-frequency-pick): Replace obsolete `case' with `pcase`.
(org--plot/radar): Replace `s-join' with `mapconcat', removing the
implicit dependency on s.el.
(org-plot/gnuplot-script): Remove unused let bindings.
(org-plot/gnuplot-script): Replace free variable reference with
expression only using given variables.
---
lisp/org-plot.el | 109 ++-
1 file changed, 52 insertions(+), 57 deletions(-)
diff --git a/lisp/org-plot.el b/lisp/org-plot.el
index 4aa8276..5c6c834 100644
--- a/lisp/org-plot.el
+++ b/lisp/org-plot.el
@@ -196,7 +196,7 @@ values, namely regarding the range."
(maximum (or hard-max (apply #'max nums)))
(range (- maximum minimum))
(rangeOrder (if (= range 0) 0
- (ceiling (- 1 (log10 range)
+ (ceiling (- 1 (log range 10)
(range-factor (expt 10 rangeOrder))
(nice-min (if (= range 0) (car nums)
(/ (float (floor (* minimum range-factor))) range-factor)))
@@ -229,34 +229,34 @@ values, namely regarding the range."
(defun org--plot/nice-frequency-pick (frequencies)
"From a list of frequences, try to sensibly pick a sample of the most frequent."
;; TODO this mosly works decently, but counld do with some tweaking to work more consistently.
- (case (length frequencies)
- (1 (list (car (nth 0 frequencies
- (2 (if (<= 3 (/ (cdr (nth 0 frequencies))
- (cdr (nth 1 frequencies
- (make-list 2
- (car (nth 0 frequencies)))
- (list (car (nth 0 frequencies))
- (car (nth 1 frequencies)
- (t
- (let* ((total-count (apply #'+ (mapcar #'cdr frequencies)))
- (n-freq (mapcar (lambda (freq) `(,(car freq) . ,(/ (float (cdr freq)) total-count))) frequencies))
- (f-pick (list (car (car n-freq
- (1-2-ratio (/ (cdr (nth 0 n-freq))
- (cdr (nth 1 n-freq
- (2-3-ratio (/ (cdr (nth 1 n-freq))
- (cdr (nth 2 n-freq
- (1-3-ratio (* 1-2-ratio 2-3-ratio))
- (1-val (car (nth 0 n-freq)))
- (2-val (car (nth 1 n-freq)))
- (3-val (car (nth 2 n-freq
- (when (> 1-2-ratio 4) (push 1-val f-pick))
- (when (and (< 1-2-ratio 2-val)
- (< (* (apply #'* f-pick) 2-val) 30))
- (push 2-val f-pick))
- (when (and (< 1-3-ratio 3-val)
- (< (* (apply #'* f-pick) 3-val) 30))
- (push 3-val f-pick))
- f-pick
+ (cl-case (length frequencies)
+(1 (list (car (nth 0 frequencies
+(2 (if (<= 3 (/ (cdr (nth 0 frequencies))
+ (cdr (nth 1 frequencies
+ (make-list 2
+ (car (nth 0 frequencies)))
+ (list (car (nth 0 frequencies))
+ (car (nth 1 frequencies)
+(t
+ (let* ((total-count (apply #'+ (mapcar #'cdr
Re: [PATCH] org-plot abstractions and extension
TEC writes:
> Kyle Meyer writes:
>
>> case is still available under the cl- prefix. If you wanted to use it
>> in 73c99bf42 (org-plot.el: add utility functions for range,ticks), I
>> don't see a reason not to use it now.
>
> I tend to use pcase over cl-case (since it's completely built in, i.e.
> no (require 'cl-lib) required).
Regardless of what you tend to use, you used "case" here in 73c99bf42;
the minimal fix is to add a cl- prefix, and any other switch with the
justification that "case is obsolete" is likely to raise a reviewer's
eyebrow.
cl-case isn't in cl-lib, and there is no need to load anything.
> I'm not sure if there's any argument for cl-case over pcase, let me
> know if so.
It depends on who you ask (and, if you search emacs.devel, you will
unsurprisingly see a range of opinions). I like pcase (and pattern
matching in general) but think it's overused in simple cases. Here's a
recent org-plot example from 8d5122fc5:
(when (pcase x
('y t)
('yes t)
('t t))
...)
That could be rewritten as
(when (memq x '(y yes t))
...)
As for the "[cl-]case -> pcase" change in this patch, I don't mind
either cl-case or pcase here. It was the switch itself that I was
commenting on. If you choose to leave it as is, that's fine.
>>> @@ -210,9 +210,9 @@ values, namely regarding the range."
>>>"From a the values in a TABLE of data, attempt to guess an appropriate
>>> number of ticks."
>>>(let* ((row-data
>>> (mapcar (lambda (row) (org--plot/values-stats
>>> - (mapcar #'string-to-number (cdr row))
>>> - hard-min
>>> - hard-max)) table))
>>> +(mapcar #'string-to-number (cdr row))
>>> +hard-min
>>> +hard-max)) table))
>>
>> Please drop this unrelated space change.
>
> Erm, this isn't unrelated. As the function being called changed length,
> the indentation of the arguments is thus also changed.
This change is in org--plot/sensible-tick-num. I don't spot any
non-whitespace changes there. Git appears to agree with me:
$ git show | grep '@@ -210,9'
@@ -210,9 +210,9 @@ (defun org--plot/sensible-tick-num (table
hard-min hard-max)
$ git show -w | grep '@@ -210,9'
> Subject: [PATCH] org-plot.el: fix compiler warnings
>
> * (org--plot/values-stats): Replace `log10' with `log'.
Please add a file name ("lisp/org-plot.el") to the start of the
changelog entry.
Re: [PATCH] org-plot abstractions and extension
Kyle Meyer writes: > case is still available under the cl- prefix. If you wanted to use it > in 73c99bf42 (org-plot.el: add utility functions for range,ticks), I > don't see a reason not to use it now. I tend to use pcase over cl-case (since it's completely built in, i.e. no (require 'cl-lib) required). I'm not sure if there's any argument for cl-case over pcase, let me know if so. > s/refence/reference/ Done >> @@ -210,9 +210,9 @@ values, namely regarding the range." >>"From a the values in a TABLE of data, attempt to guess an appropriate >> number of ticks." >>(let* ((row-data >>(mapcar (lambda (row) (org--plot/values-stats >> -(mapcar #'string-to-number (cdr row)) >> -hard-min >> -hard-max)) table)) >> + (mapcar #'string-to-number (cdr row)) >> + hard-min >> + hard-max)) table)) > > Please drop this unrelated space change. Erm, this isn't unrelated. As the function being called changed length, the indentation of the arguments is thus also changed. > The mapcar is unnecessary; you can reposition (lambda ...) as > mapconcat's FUNCTION argument. Thanks for spotting that. Resolved. Updated patch attached. Let me know how it looks to you :) -- Timothy >From 22717d0750e2c001003b45f1d4834571f21287ef Mon Sep 17 00:00:00 2001 From: TEC Date: Wed, 23 Dec 2020 14:13:24 +0800 Subject: [PATCH] org-plot.el: fix compiler warnings * (org--plot/values-stats): Replace `log10' with `log'. (org--plot/nice-frequency-pick): Replace obsolete `case' with `pcase`. (org--plot/radar): Replace `s-join' with `mapconcat', removing the implicit dependency on s.el. (org-plot/gnuplot-script): Remove unused let bindings. (org-plot/gnuplot-script): Replace free variable reference with expression only using given variables. --- lisp/org-plot.el | 115 +++ 1 file changed, 55 insertions(+), 60 deletions(-) diff --git a/lisp/org-plot.el b/lisp/org-plot.el index 4aa8276..1c7ee43 100644 --- a/lisp/org-plot.el +++ b/lisp/org-plot.el @@ -196,7 +196,7 @@ values, namely regarding the range." (maximum (or hard-max (apply #'max nums))) (range (- maximum minimum)) (rangeOrder (if (= range 0) 0 - (ceiling (- 1 (log10 range) + (ceiling (- 1 (log range 10) (range-factor (expt 10 rangeOrder)) (nice-min (if (= range 0) (car nums) (/ (float (floor (* minimum range-factor))) range-factor))) @@ -210,9 +210,9 @@ values, namely regarding the range." "From a the values in a TABLE of data, attempt to guess an appropriate number of ticks." (let* ((row-data (mapcar (lambda (row) (org--plot/values-stats - (mapcar #'string-to-number (cdr row)) - hard-min - hard-max)) table)) + (mapcar #'string-to-number (cdr row)) + hard-min + hard-max)) table)) (row-normalised-ranges (mapcar (lambda (r-data) (let ((val (round (* (plist-get r-data :range-factor) @@ -229,34 +229,34 @@ values, namely regarding the range." (defun org--plot/nice-frequency-pick (frequencies) "From a list of frequences, try to sensibly pick a sample of the most frequent." ;; TODO this mosly works decently, but counld do with some tweaking to work more consistently. - (case (length frequencies) - (1 (list (car (nth 0 frequencies - (2 (if (<= 3 (/ (cdr (nth 0 frequencies)) - (cdr (nth 1 frequencies - (make-list 2 - (car (nth 0 frequencies))) - (list (car (nth 0 frequencies)) - (car (nth 1 frequencies) - (t - (let* ((total-count (apply #'+ (mapcar #'cdr frequencies))) - (n-freq (mapcar (lambda (freq) `(,(car freq) . ,(/ (float (cdr freq)) total-count))) frequencies)) - (f-pick (list (car (car n-freq - (1-2-ratio (/ (cdr (nth 0 n-freq)) - (cdr (nth 1 n-freq - (2-3-ratio (/ (cdr (nth 1 n-freq)) - (cdr (nth 2 n-freq - (1-3-ratio (* 1-2-ratio 2-3-ratio)) - (1-val (car (nth 0 n-freq))) - (2-val (car (nth 1 n-freq))) - (3-val (car (nth 2 n-freq - (when (> 1-2-ratio 4) (push 1-val f-pick)) - (when (and (< 1-2-ratio 2-val) - (< (* (apply #'* f-pick) 2-val) 30)) - (push 2-val f-pick)) - (when (and (< 1-3-ratio 3-val) - (< (* (apply #'* f-pick) 3-val) 30)) - (push 3-val f-pick)) - f-pick + (pcase (length frequencies) +(1 (list (car (nth 0 frequencies +(2 (if (<= 3 (/ (cdr (nth 0 frequencies)) + (cdr (nth 1 frequencies + (make-list 2 + (car (nth 0 frequencies))) + (list (car (nth 0 frequencies)) + (car (nth 1 frequencies) +(_ + (let* ((total-count (apply #'+ (mapcar #'cdr frequencies))) + (n-freq (mapcar (lambda (freq) `(,(car freq) . ,(/ (float (cdr freq)) total-count))) frequencies)) + (f-pick (list (car (car n-freq + (1-2-ratio (/ (cdr (nth 0 n-freq)) + (cdr (nth 1 n-freq +
Re: [PATCH] org-plot abstractions and extension
> Subject: [PATCH] org-plot.el: fix compiler warnings > > * (org--plot/values-stats): Replace `log10' with `log'. > (org--plot/nice-frequency-pick): Replace obsolete `case' with `pcase`. case is still available under the cl- prefix. If you wanted to use it in 73c99bf42 (org-plot.el: add utility functions for range,ticks), I don't see a reason not to use it now. > (org--plot/radar): Replace `s-join' with `mapconcat', removing the > implicit dependency on s.el. > (org-plot/gnuplot-script): Remove unused let bindings. > (org-plot/gnuplot-script): Replace free variable refence with expression s/refence/reference/ > @@ -210,9 +210,9 @@ values, namely regarding the range." >"From a the values in a TABLE of data, attempt to guess an appropriate > number of ticks." >(let* ((row-data > (mapcar (lambda (row) (org--plot/values-stats > - (mapcar #'string-to-number (cdr row)) > - hard-min > - hard-max)) table)) > + (mapcar #'string-to-number (cdr row)) > + hard-min > + hard-max)) table)) Please drop this unrelated space change. > @@ -473,34 +473,36 @@ EOD > > (defun org--plot/radar (table params) >(let* ((data > - (concat "\"" (s-join "\" \"" (plist-get params :labels)) "\"" > + (concat "\"" (mapconcat #'identity (plist-get params :labels) "\" > \"") "\"" > "\n" > - (s-join "\n" > - (mapcar (lambda (row) > - (format > - "\"%s\" %s" > - (car row) > - (s-join " " (cdr row > - (append table (list (car table))) > + (mapconcat #'identity > + (mapcar (lambda (row) > +(format > + "\"%s\" %s" > + (car row) > + (mapconcat #'identity (cdr row) " "))) > + (append table (list (car table > + "\n"))) The mapcar is unnecessary; you can reposition (lambda ...) as mapconcat's FUNCTION argument. Same comment applies to a spot farther down in the patch. Thanks.
Re: [PATCH] org-plot abstractions and extension
TEC writes: > Kyle Meyer writes: > >> This series introduced some compiler warnings. >> >> Timothy, could you please submit a follow-up patch to address these? > > Absolutely. Thanks for raising this, I'll take a look shortly. Here we go :) >From 309907af5e76818753b85af84b3e304d8cb4568c Mon Sep 17 00:00:00 2001 From: TEC Date: Wed, 23 Dec 2020 14:13:24 +0800 Subject: [PATCH] org-plot.el: fix compiler warnings * (org--plot/values-stats): Replace `log10' with `log'. (org--plot/nice-frequency-pick): Replace obsolete `case' with `pcase`. (org--plot/radar): Replace `s-join' with `mapconcat', removing the implicit dependency on s.el. (org-plot/gnuplot-script): Remove unused let bindings. (org-plot/gnuplot-script): Replace free variable refence with expression only using given variables. --- lisp/org-plot.el | 117 +++ 1 file changed, 57 insertions(+), 60 deletions(-) diff --git a/lisp/org-plot.el b/lisp/org-plot.el index 4aa8276..80700e0 100644 --- a/lisp/org-plot.el +++ b/lisp/org-plot.el @@ -196,7 +196,7 @@ values, namely regarding the range." (maximum (or hard-max (apply #'max nums))) (range (- maximum minimum)) (rangeOrder (if (= range 0) 0 - (ceiling (- 1 (log10 range) + (ceiling (- 1 (log range 10) (range-factor (expt 10 rangeOrder)) (nice-min (if (= range 0) (car nums) (/ (float (floor (* minimum range-factor))) range-factor))) @@ -210,9 +210,9 @@ values, namely regarding the range." "From a the values in a TABLE of data, attempt to guess an appropriate number of ticks." (let* ((row-data (mapcar (lambda (row) (org--plot/values-stats - (mapcar #'string-to-number (cdr row)) - hard-min - hard-max)) table)) + (mapcar #'string-to-number (cdr row)) + hard-min + hard-max)) table)) (row-normalised-ranges (mapcar (lambda (r-data) (let ((val (round (* (plist-get r-data :range-factor) @@ -229,34 +229,34 @@ values, namely regarding the range." (defun org--plot/nice-frequency-pick (frequencies) "From a list of frequences, try to sensibly pick a sample of the most frequent." ;; TODO this mosly works decently, but counld do with some tweaking to work more consistently. - (case (length frequencies) - (1 (list (car (nth 0 frequencies - (2 (if (<= 3 (/ (cdr (nth 0 frequencies)) - (cdr (nth 1 frequencies - (make-list 2 - (car (nth 0 frequencies))) - (list (car (nth 0 frequencies)) - (car (nth 1 frequencies) - (t - (let* ((total-count (apply #'+ (mapcar #'cdr frequencies))) - (n-freq (mapcar (lambda (freq) `(,(car freq) . ,(/ (float (cdr freq)) total-count))) frequencies)) - (f-pick (list (car (car n-freq - (1-2-ratio (/ (cdr (nth 0 n-freq)) - (cdr (nth 1 n-freq - (2-3-ratio (/ (cdr (nth 1 n-freq)) - (cdr (nth 2 n-freq - (1-3-ratio (* 1-2-ratio 2-3-ratio)) - (1-val (car (nth 0 n-freq))) - (2-val (car (nth 1 n-freq))) - (3-val (car (nth 2 n-freq - (when (> 1-2-ratio 4) (push 1-val f-pick)) - (when (and (< 1-2-ratio 2-val) - (< (* (apply #'* f-pick) 2-val) 30)) - (push 2-val f-pick)) - (when (and (< 1-3-ratio 3-val) - (< (* (apply #'* f-pick) 3-val) 30)) - (push 3-val f-pick)) - f-pick + (pcase (length frequencies) +(1 (list (car (nth 0 frequencies +(2 (if (<= 3 (/ (cdr (nth 0 frequencies)) + (cdr (nth 1 frequencies + (make-list 2 + (car (nth 0 frequencies))) + (list (car (nth 0 frequencies)) + (car (nth 1 frequencies) +(_ + (let* ((total-count (apply #'+ (mapcar #'cdr frequencies))) + (n-freq (mapcar (lambda (freq) `(,(car freq) . ,(/ (float (cdr freq)) total-count))) frequencies)) + (f-pick (list (car (car n-freq + (1-2-ratio (/ (cdr (nth 0 n-freq)) + (cdr (nth 1 n-freq + (2-3-ratio (/ (cdr (nth 1 n-freq)) + (cdr (nth 2 n-freq + (1-3-ratio (* 1-2-ratio 2-3-ratio)) + (1-val (car (nth 0 n-freq))) + (2-val (car (nth 1 n-freq))) + (3-val (car (nth 2 n-freq + (when (> 1-2-ratio 4) (push 1-val f-pick)) + (when (and (< 1-2-ratio 2-val) + (< (* (apply #'* f-pick) 2-val) 30)) + (push 2-val f-pick)) + (when (and (< 1-3-ratio 3-val) + (< (* (apply #'* f-pick) 3-val) 30)) + (push 3-val f-pick)) + f-pick (defun org--plot/merge-alists (function default alist1 alist2 alists) "Using FUNCTION, combine the elements of all given ALISTS. When an element is @@ -473,34 +473,36 @@ EOD (defun org--plot/radar (table params) (let* ((data - (concat "\"" (s-join "\" \"" (plist-get params :labels)) "\"" + (concat "\"" (mapconcat #'identity (plist-get params :labels) "\" \"") "\"" "\n" - (s-join "\n" - (mapcar (lambda (row) -(format - "\"%s\" %s" - (car row) - (s-join " " (cdr row - (append table (list (car table))) + (mapconcat #'identity +
Re: [PATCH] org-plot abstractions and extension
Kyle Meyer writes: > This series introduced some compiler warnings. > > Timothy, could you please submit a follow-up patch to address these? Absolutely. Thanks for raising this, I'll take a look shortly. -- Timothy
Re: [PATCH] org-plot abstractions and extension
Bastien writes: > Hi Timothy, > > TEC writes: > >> I'll attach all the patches to this email, so there's no ambiguity. >> (crosses fingers for attachments working as expected) > > Applied, with minor modifications of the changelog entries:[...] This series introduced some compiler warnings. $ make compile [...] In org--plot/values-stats: org-plot.el:199:39:Warning: ‘log10’ is an obsolete function (as of 24.4); use ‘log’ instead. In org--plot/nice-frequency-pick: org-plot.el:229:39:Warning: ‘1’ is a malformed function org-plot.el:229:39:Warning: ‘2’ is a malformed function org-plot.el:239:32:Warning: ‘t’ called as a function org-plot.el:525:1:Warning: Unused lexical variable ‘col-labels’ org-plot.el:525:1:Warning: Unused lexical variable ‘deps’ org-plot.el:525:1:Warning: Unused lexical variable ‘text-ind’ org-plot.el:525:1:Warning: Unused lexical variable ‘ind’ org-plot.el:525:1:Warning: Unused lexical variable ‘map’ In org-plot/gnuplot: org-plot.el:653:56:Warning: reference to free variable ‘type-name’ In end of data: org-plot.el:706:1:Warning: the following functions are not known to be defined: case, t, s-join Most of these are minor (case -> cl-case, dropping unused bindings, ...). The s-join one is the most important because s.el is not a dependency. Timothy, could you please submit a follow-up patch to address these?
Re: [PATCH] org-plot abstractions and extension
TEC writes: > Thanks for the feedback on the patches! I tried to get it right after my > previous (and first) attempt, but it looks like there are a few things I > still need to take note of. Big improvement though, nonetheless, > hopefully next time they'll be nothing to change :) Well, It's perfectly fine not to send a perfect patch. Thanks! -- Bastien
Re: [PATCH] org-plot abstractions and extension
Bastien writes: > Applied, with minor modifications of the changelog entries: > > - You need to add an entry when creating a new custom variable. > > - I suggest saying "option" instead of "custom variable". > > - Sentences should be separated by two spaces and start with an > uppercase letter. > > Also, the convention in Emacs is to avoid whitespaces-only commits, > you need to fix whitespaces within other non-whitespaces changes in > a commit. Thanks for the feedback on the patches! I tried to get it right after my previous (and first) attempt, but it looks like there are a few things I still need to take note of. Big improvement though, nonetheless, hopefully next time they'll be nothing to change :) All the best, Timothy
Re: [PATCH] org-plot abstractions and extension
Hi Timothy, TEC writes: > I'll attach all the patches to this email, so there's no ambiguity. > (crosses fingers for attachments working as expected) Applied, with minor modifications of the changelog entries: - You need to add an entry when creating a new custom variable. - I suggest saying "option" instead of "custom variable". - Sentences should be separated by two spaces and start with an uppercase letter. Also, the convention in Emacs is to avoid whitespaces-only commits, you need to fix whitespaces within other non-whitespaces changes in a commit. Thanks a lot! -- Bastien
Re: [PATCH] org-plot abstractions and extension
Hi Timothy, TEC writes: > It's now been 1.5 months, so I'm going to bump this thread. Sure - thanks for bumping this. I'm slowly reading emails from the past weeks, I will handle this ASAP. Thanks, -- Bastien
Re: [PATCH] org-plot abstractions and extension
It's now been 1.5 months, so I'm going to bump this thread. -- Timothy TEC writes: > Bastien writes: > >> I'm not an org-plot.el user so I cannot test, but by reading the >> patches, they look okay. >> >> Let's go in "optimistic merging" mode and commit your patches? > > Sounds good then. I don't expect the changes to compromise any > existing > functionality. > >> Is https://orgmode.org/list/87lfhbhfhe@gmail.com/ the latest >> version I should use? > > I've smoothed a rough edge or two, and added a documentation > entry. > > I'll attach all the patches to this email, so there's no > ambiguity. > (crosses fingers for attachments working as expected)
Re: [PATCH] org-plot abstractions and extension
On Sat, Oct 24, 2020 at 2:21 PM TEC wrote: > Sounds good then. I don't expect the changes to compromise any > existing > functionality. I tested these patches and didn't have a problem. I didn't go out of my way to see what changed and test it, but org-plot didn't break for what I was doing. I was hoping to be able to change line colors, but didn't find a way. I'll ask about it in another thread.
Re: [PATCH] org-plot abstractions and extension
Bastien writes:
I'm not an org-plot.el user so I cannot test, but by reading the
patches, they look okay.
Let's go in "optimistic merging" mode and commit your patches?
Sounds good then. I don't expect the changes to compromise any
existing
functionality.
Is https://orgmode.org/list/87lfhbhfhe@gmail.com/ the latest
version I should use?
I've smoothed a rough edge or two, and added a documentation
entry.
I'll attach all the patches to this email, so there's no
ambiguity.
(crosses fingers for attachments working as expected)
--
Timothy
>From 3743e507775b446f5f8188958c20f65861fac3fb Mon Sep 17 00:00:00 2001
From: TEC
Date: Wed, 8 Jul 2020 18:34:46 +0800
Subject: [PATCH 01/15] org-plot.el: make indentation method consistent
* lisp/org-plot.el (org-plot/gnuplot): Make indentation consistent, by
replacing a few spaces with tabs.
Only 6 of 347 lines used spaces instead of tabs.
---
lisp/org-plot.el | 12 ++--
1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/lisp/org-plot.el b/lisp/org-plot.el
index 0ff96af67..c08bc144e 100644
--- a/lisp/org-plot.el
+++ b/lisp/org-plot.el
@@ -325,12 +325,12 @@ line directly before or after the table."
(with-temp-buffer
(if (plist-get params :script) ; user script
(progn (insert
-(org-plot/gnuplot-script data-file num-cols params t))
- (insert "\n")
- (insert-file-contents (plist-get params :script))
- (goto-char (point-min))
- (while (re-search-forward "\\$datafile" nil t)
- (replace-match data-file nil nil)))
+ (org-plot/gnuplot-script data-file num-cols params t))
+ (insert "\n")
+ (insert-file-contents (plist-get params :script))
+ (goto-char (point-min))
+ (while (re-search-forward "\\$datafile" nil t)
+ (replace-match data-file nil nil)))
(insert (org-plot/gnuplot-script data-file num-cols params)))
;; Graph table.
(gnuplot-mode)
--
2.28.0
>From c62e817b04dfbe624ee8b2090ebcde257bbd3f23 Mon Sep 17 00:00:00 2001
From: TEC
Date: Wed, 8 Jul 2020 19:26:07 +0800
Subject: [PATCH 02/15] org-plot.el: add new option :transpose
* lisp/org-plot.el (org-plot/add-options-to-plist,
org-plot/add-options-to-plist): Add a new option :transpose, and a
shorter alias :trans. Transposition is performed if the argument is yes,
y, or t. This treats the table as a matrix and performs matrix
transposition on it. If an hline is present, it is assumed that it is a
marks a separation from a first header row. The first row is then
treated as the new header by inserting a hline in the transposed data.
This is quite useful for some plots, where across multiple categories,
there are a large number of data points. Without this, the data points
would be columns and the table can spread irritatingly wide.
---
lisp/org-plot.el | 44 +---
1 file changed, 29 insertions(+), 15 deletions(-)
diff --git a/lisp/org-plot.el b/lisp/org-plot.el
index c08bc144e..6ff633130 100644
--- a/lisp/org-plot.el
+++ b/lisp/org-plot.el
@@ -50,19 +50,21 @@
"Parse an OPTIONS line and set values in the property list P.
Returns the resulting property list."
(when options
-(let ((op '(("type". :plot-type)
- ("script" . :script)
- ("line". :line)
- ("set" . :set)
- ("title" . :title)
- ("ind" . :ind)
- ("deps". :deps)
- ("with". :with)
- ("file". :file)
- ("labels" . :labels)
- ("map" . :map)
- ("timeind" . :timeind)
- ("timefmt" . :timefmt)))
+(let ((op '(("type" . :plot-type)
+ ("script". :script)
+ ("line" . :line)
+ ("set" . :set)
+ ("title" . :title)
+ ("ind" . :ind)
+ ("deps" . :deps)
+ ("with" . :with)
+ ("file" . :file)
+ ("labels". :labels)
+ ("map" . :map)
+ ("timeind" . :timeind)
+ ("timefmt" . :timefmt)
+ ("trans" . :transpose)
+ ("transpose" . :transpose)))
(multiples '("set" "line"))
(regexp ":\\([\"][^\"]+?[\"]\\|[(][^)]+?[)]\\|[^ \t\n\r;,.]*\\)")
(start 0))
@@ -289,8 +291,20 @@ line directly before or after the table."
(setf params (plist-put params (car pair) (cdr pair)
;; collect table and table information
(let* ((data-file (make-temp-file "org-plot"))
- (table (org-table-collapse-header (org-table-to-lisp)))
- (num-cols (length (car table
+ (table (let ((tbl (org-table-to-lisp)))
+ (when (pcase (plist-get params :transpose)
+ ('y t)
+ ('yes t)
+ ('t t))
+ (if (memq 'hline tbl)
+ (setq tbl (apply #'cl-mapcar #'list tbl))
+ ;; When present, remove hlines as they can't (currentily) be easily transposed.
+ (setq tbl (apply #'cl-mapcar #'list
+ (remove 'hline tbl)))
+ (push 'hline (cdr tbl
+ tbl))
+ (num-cols (length (if (eq (nth 0 table) 'hline) (nth 1 table)
+ (nth 0 table)
(run-with-idle-timer 0.1 nil
Re: [PATCH] org-plot abstractions and extension
Hi Timothy, TEC writes: > I'm still hoping that someone might get back to me ... eventually, > so here's another bump. I'm not an org-plot.el user so I cannot test, but by reading the patches, they look okay. Let's go in "optimistic merging" mode and commit your patches? Is https://orgmode.org/list/87lfhbhfhe@gmail.com/ the latest version I should use? -- Bastien
Re: [PATCH] org-plot abstractions and extension
Hello all, I'm still hoping that someone might get back to me ... eventually, so here's another bump. Timothy. TEC writes: Hello everyone. Just in case this has slipped through the cracks / fallen under the radar --- here's a little bump. Timothy. TEC writes: Oooops, I've just noticed my patch attachment re-send was only addressed to Bastien (maybe this is why I haven't heard anything?). This is what I get for mixing mail clients and not paying attention I guess . If someone would be willing to have a look through my work, and comment - that would be fantastic. I'd love to get my code into shape to be merged :) All the best, Timothy.
Re: [PATCH] org-plot abstractions and extension
Hello everyone. Just in case this has slipped through the cracks / fallen under the radar --- here's a little bump. Timothy. TEC writes: > Oooops, I've just noticed my patch attachment re-send was only addressed > to Bastien (maybe this is why I haven't heard anything?). > This is what I get for mixing mail clients and not paying attention > I guess . > > If someone would be willing to have a look through my work, and comment > - that would be fantastic. > > I'd love to get my code into shape to be merged :) > > All the best, > > Timothy.
Re: [PATCH] org-plot abstractions and extension
Oooops, I've just noticed my patch attachment re-send was only addressed
to Bastien (maybe this is why I haven't heard anything?).
This is what I get for mixing mail clients and not paying attention
I guess .
If someone would be willing to have a look through my work, and comment
- that would be fantastic.
I'd love to get my code into shape to be merged :)
All the best,
Timothy.
> Bastien wrote:
>
>> Can you repost as plain text? The email is not very readable in HTML
>> and the patches are not readable at all.
>
> Ooops, that shouldn't have happened. Unfortunately, I have yet to find
> a good way of attaching files in mu4e.
> Those should have been converted into attachments in a plaintext
> email, but that didn't work.
>
> Let me know if this attempt works as intended,
>
> Timothy.
>From c62e817b04dfbe624ee8b2090ebcde257bbd3f23 Mon Sep 17 00:00:00 2001
From: TEC
Date: Wed, 8 Jul 2020 19:26:07 +0800
Subject: [PATCH 02/11] org-plot.el: add new option :transpose
* lisp/org-plot.el (org-plot/add-options-to-plist,
org-plot/add-options-to-plist): Add a new option :transpose, and a
shorter alias :trans. Transposition is performed if the argument is yes,
y, or t. This treats the table as a matrix and performs matrix
transposition on it. If an hline is present, it is assumed that it is a
marks a separation from a first header row. The first row is then
treated as the new header by inserting a hline in the transposed data.
This is quite useful for some plots, where across multiple categories,
there are a large number of data points. Without this, the data points
would be columns and the table can spread irritatingly wide.
---
lisp/org-plot.el | 44 +---
1 file changed, 29 insertions(+), 15 deletions(-)
diff --git a/lisp/org-plot.el b/lisp/org-plot.el
index c08bc144e..6ff633130 100644
--- a/lisp/org-plot.el
+++ b/lisp/org-plot.el
@@ -50,19 +50,21 @@
"Parse an OPTIONS line and set values in the property list P.
Returns the resulting property list."
(when options
-(let ((op '(("type". :plot-type)
- ("script" . :script)
- ("line". :line)
- ("set" . :set)
- ("title" . :title)
- ("ind" . :ind)
- ("deps". :deps)
- ("with". :with)
- ("file". :file)
- ("labels" . :labels)
- ("map" . :map)
- ("timeind" . :timeind)
- ("timefmt" . :timefmt)))
+(let ((op '(("type" . :plot-type)
+ ("script". :script)
+ ("line" . :line)
+ ("set" . :set)
+ ("title" . :title)
+ ("ind" . :ind)
+ ("deps" . :deps)
+ ("with" . :with)
+ ("file" . :file)
+ ("labels". :labels)
+ ("map" . :map)
+ ("timeind" . :timeind)
+ ("timefmt" . :timefmt)
+ ("trans" . :transpose)
+ ("transpose" . :transpose)))
(multiples '("set" "line"))
(regexp ":\\([\"][^\"]+?[\"]\\|[(][^)]+?[)]\\|[^ \t\n\r;,.]*\\)")
(start 0))
@@ -289,8 +291,20 @@ line directly before or after the table."
(setf params (plist-put params (car pair) (cdr pair)
;; collect table and table information
(let* ((data-file (make-temp-file "org-plot"))
- (table (org-table-collapse-header (org-table-to-lisp)))
- (num-cols (length (car table
+ (table (let ((tbl (org-table-to-lisp)))
+ (when (pcase (plist-get params :transpose)
+ ('y t)
+ ('yes t)
+ ('t t))
+ (if (memq 'hline tbl)
+ (setq tbl (apply #'cl-mapcar #'list tbl))
+ ;; When present, remove hlines as they can't (currentily) be easily transposed.
+ (setq tbl (apply #'cl-mapcar #'list
+ (remove 'hline tbl)))
+ (push 'hline (cdr tbl
+ tbl))
+ (num-cols (length (if (eq (nth 0 table) 'hline) (nth 1 table)
+ (nth 0 table)
(run-with-idle-timer 0.1 nil #'delete-file data-file)
(when (eq (cadr table) 'hline)
(setf params
--
2.28.0
>From fc7f4015c726e4a685002e8d69fad1eb1d605790 Mon Sep 17 00:00:00 2001
From: TEC
Date: Wed, 8 Jul 2020 22:26:21 +0800
Subject: [PATCH 03/11] org-plot.el: add new custom gnuplot preamble
* lisp/org-plot.el: Define new custom variable
`org-plot/gnuplot-script-preamble' which can be either a string or a
function. The value of this (when executed, in the case of the
function) is inserted near the top of the generated gnuplot script.
(org-plot/gnuplot-script): Use the new variable
`org-plot/gnuplot-script-preamble' in the manner described.
This allows for the user to set the font/colour-scheme, default
precision, and much more.
---
lisp/org-plot.el | 13 +
1 file changed, 13 insertions(+)
diff --git a/lisp/org-plot.el b/lisp/org-plot.el
index 6ff633130..f8db45273 100644
--- a/lisp/org-plot.el
+++ b/lisp/org-plot.el
@@ -181,6 +181,13 @@ and dependent variables."
(setf back-edge "") (setf front-edge ""
row-vals))
+(defcustom org-plot/gnuplot-script-preamble ""
+ "String or function which provides content to be inserted into the GNUPlot
+script before the plot command. Not that this
