From: Mario Frasca <firstname.lastname@example.org>
To: Kyle Meyer <email@example.com>, firstname.lastname@example.org
Subject: Re: `with` as a list.
Date: Sat, 30 May 2020 09:23:21 -0500 [thread overview]
Message-ID: <email@example.com> (raw)
thank you for writing back, I have a couple of questions in reply.
btw. are you on irc.freenode.net? I'm `mariotomo' there. and I just
joined `#org-mode'. I don't think that my terminal will ring a bell if
I'm mentioned there.
On 30/05/2020 01:22, Kyle Meyer wrote:
> Mario Frasca writes:
> Thanks for the patch and for clearly describing the motivation. I'm not
> an org-plot user, but the change sounds useful. (It'd be great if
> org-plot users could chime in.)
thank you for taking the time to review!
> Two meta-comments:
> * Please provide a patch with a commit message. See
> <https://orgmode.org/worg/org-contribute.html> for more information.
> * The link above also explains the copyright assignment requirement
> for contributions. Please consider assigning copyright to the FSF.
I'm editing in my cloned repository, and committing often, so I do not
have a single commit, consequently also not a single commit message.
I just sent my form request to firstname.lastname@example.org.
>> diff --git a/lisp/org-plot.el b/lisp/org-plot.el
>> index a23195d2a..87a415137 100644
>> --- a/lisp/org-plot.el
>> +++ b/lisp/org-plot.el
>> @@ -179,6 +179,28 @@ and dependent variables."
>> (setf back-edge "") (setf front-edge ""))))
>> +(defun org-plot/zip-deps-with (num-cols ind deps with)
>> + "describe each column to be plotted as (col . with)"
> This doesn't match the convention used in this code base for docstrings.
> Taking a look around should give you a good sense, but (1) the first
> word should be capitalized, (2) sentences should end with a period, and
> (3) ideally all parameters should be described in the docstring.
ok, 1 and 2 are just consequence of my usual way of typing, however
wrong, I will fix them. 3 I didn't consider. useful point.
>> + ;; make 'deps explicit
> I think this and the other comments in this function could safely be
>> + (unless deps
>> + (setf deps (let (r)
>> + (dotimes (i num-cols r)
>> + (unless (eq num-cols (+ ind i))
>> + (setq r (cons (- num-cols i) r)))))))
> […] using setq unless setf is needed would be more
> consistent with this code base.
the "unless needed" makes me suspect I should read what's the difference.
> The code above looks fine to me. Another option would be to use
> number-sequence and then filter out the ind value.
no, that would break functionality: I need to keep the given order.
btw my patch allows you to use the same column more than once.
>> + ;; invoke zipping function on converted data
>> + (org-plot/zip deps with))
>> +(defun org-plot/zip (xs ys)
>> + (unless
>> + (null xs)
>> + (cons (cons (car xs) (or (car ys) "lines"))
> Is the "lines" fall back ever reached? It looks like you're extending
> `with' above to be the same length as `deps`.
it is needed: I'm not extending any `with' given as a non-empty list.
but I should be using some settings, some global default `with' value,
which I don't know where to find. hard coding "lines" can't be the
right thing to do.
>> + (org-plot/zip (cdr xs) (cdr ys)))))
> In Elisp, it's not very common to use recursive functions (and there's
> no TCO). In the case of zipping two lists, I think it'd probably be
> easiest to just use cl-loop. And in my view having a separate function
> here is probably an overkill.
I'm not sure about the TCO (in other words: I haven't the faintest idea
what you mean by that), and I would not know how to write this using
`cl-loop'. I'll have a look though.
>> (defun org-plot/gnuplot-script (data-file num-cols params &optional preface)
>> "Write a gnuplot script to DATA-FILE respecting the options set in PARAMS.
>> NUM-COLS controls the number of columns plotted in a 2-d plot.
>> @@ -240,22 +262,22 @@ manner suitable for prepending to a user-specified script."
>> "%Y-%m-%d-%H:%M:%S") "\"")))
>> (unless preface
>> (pcase type ; plot command
>> - (`2d (dotimes (col num-cols)
>> - (unless (and (eq type '2d)
>> - (or (and ind (equal (1+ col) ind))
>> - (and deps (not (member (1+ col) deps)))))
>> - (setf plot-lines
>> - (cons
>> - (format plot-str data-file
>> - (or (and ind (> ind 0)
>> - (not text-ind)
>> - (format "%d:" ind)) "")
>> - (1+ col)
>> - (if text-ind (format ":xticlabel(%d)" ind) "")
>> - with
>> - (or (nth col col-labels)
>> - (format "%d" (1+ col))))
>> - plot-lines)))))
>> + (`2d (dolist
>> + (col-with
>> + (org-plot/zip-deps-with num-cols ind deps with))
>> + (setf plot-lines
>> + (cons
>> + (format plot-str data-file
>> + (or (and ind (> ind 0)
>> + (not text-ind)
>> + (format "%d:" ind)) "")
>> + (car col-with)
>> + (if text-ind (format ":xticlabel(%d)" ind) "")
>> + (cdr col-with)
>> + (or (nth (1- (car col-with))
>> + col-labels)
>> + (format "%d" (car col-with))))
>> + plot-lines))))
> I haven't looked at this bit too closely (and I know the handling around
> col-labels changed a bit in the last message you sent), but I did try
> out a few org-plot invocations and things seemed to work as I expected.
> I'll plan to take a closer when you send a reroll.
the whole org-plot.el has (had) no unit tests, so if you share a couple
of your org-plot invocations, I can convert them to regression tests.
>> @@ -320,7 +343,7 @@ line directly before or after the table."
>> (plist-put params :timeind t)
>> ;; Check for text ind column.
>> - (if (or (string= (plist-get params :with) "hist")
>> + (if (or (and (stringp with) (string= with "hist"))
> Could be simplified by using `equal'.
yes, definitely! as said, I'm not a lisp programmer. ;-)
I hope to post a new diff soon.
next prev parent reply other threads:[~2020-05-30 14:24 UTC|newest]
Thread overview: 23+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-05-22 16:07 `with` as a list Mario Frasca
2020-05-28 13:34 ` [PATCH] [FEATURE] " Mario Frasca
2020-05-30 6:22 ` Kyle Meyer
2020-05-30 14:23 ` Mario Frasca [this message]
2020-05-30 14:38 ` Mario Frasca
2020-05-30 20:23 ` Kyle Meyer
2020-05-30 21:29 ` Mario Frasca
2020-05-31 20:18 ` Mario Frasca
2020-06-01 0:19 ` Kyle Meyer
2020-06-01 1:47 ` Mario Frasca
2020-06-01 2:31 ` Kyle Meyer
2020-06-03 15:09 ` Mario Frasca
2020-06-03 15:13 ` Bastien
2020-06-03 15:18 ` Mario Frasca
2020-06-03 15:29 ` Bastien
2020-06-03 17:08 ` Mario Frasca
2020-06-03 22:15 ` Mario Frasca
2020-06-03 15:25 ` Mario Frasca
2020-06-03 15:30 ` Bastien
2020-05-30 16:01 ` Mario Frasca
2020-05-30 20:25 ` Kyle Meyer
2020-05-30 21:36 ` Mario Frasca
2020-05-31 0:36 ` Kyle Meyer
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:
List information: https://www.orgmode.org/
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).