Mario Frasca writes: > diff --git a/lisp/org-plot.el b/lisp/org-plot.el > index a23195d2a..c44cca991 100644 > --- a/lisp/org-plot.el > +++ b/lisp/org-plot.el > @@ -156,7 +156,8 @@ and dependent variables." > table))) > ;; write table to gnuplot grid datafile format > (with-temp-file data-file > - (let ((num-rows (length table)) (num-cols (length (nth 0 table))) > + (let ((num-rows (length table)) > + (num-cols (length (nth 0 table)))
I don't disagree with the style preference here, but it's easier on reviewers if the patch doesn't contain unrelated changes. > (gnuplot-row (lambda (col row value) > (setf col (+ 1 col)) (setf row (+ 1 row)) > (format "%f %f %f\n%f %f %f\n" > @@ -179,6 +180,22 @@ and dependent variables." > (setf back-edge "") (setf front-edge "")))) > row-vals)) > > +(defun org-plot/zip-deps-with (num-cols ind deps with) > + "Describe each column to be plotted as (col . with). > +Loops over DEPS and WITH in order to cons their elements. > +If the DEPS list of columns is not given, use all columns from 1 > +to NUM-COLS, excluding IND. > +If WITH is given as a string, use the given value for all > +columns. > +If WITH is given as a list, and it's shorter than DEPS, expand it > +with the global default value." Thanks for updating the docstring. > + (unless deps > + (setq deps (remove ind (number-sequence 1 num-cols)))) > + (unless (listp with) > + (setq with (make-list (length deps) with))) > + (setq with (append with (make-list (- (length deps) (length with)) > "lines"))) The caller can crash this with something like make-list(-1 "lines") (append with (make-list (- (length deps) (length with)) "lines")) (setq with (append with (make-list (- (length deps) (length with)) "lines"))) org-plot/zip-deps-with(3 1 (2 3) (lines points lines)) [...] if they accidentally give more `with` values than `deps`. Also, if the `(unless (listp with) ...` condition is entered, I think the second make-list call is unnecessary. So, combining those two points, perhaps something like this: (setq with (if (listp with) (append with (make-list (max 0 (- (length deps) (length with))) "lines")) (make-list (length deps) with))) > + (cl-mapcar 'cons deps with)) Nit-pick: s/'/#'/ The latter is short for `function' and, unlike the former, lets the byte-compiler know that cons is a function (enabling a warning if, say, you accidentally typed `conss`). > (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 +257,24 @@ 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) > + (apply (lambda (x) > + (if (= 0 (length x)) > + (format "%d" (car col-with)) > + x)) > + (list (nth (1- (car col-with)) > col-labels)))) If I'm reading it correctly, (apply ...) could be simplified to (or (nth (1- (car col-with)) col-labels) (format "%d" (car col-with))) > diff --git a/testing/lisp/test-org-plot.el b/testing/lisp/test-org-plot.el > new file mode 100644 > index 000000000..2bbd09b5f > --- /dev/null > +++ b/testing/lisp/test-org-plot.el > @@ -0,0 +1,50 @@ > +;;; test-org-plot.el --- Tests for org-plot.el > + > +;; Copyright (C) 2020 Mario Frasca > + > +;; Author: Mario Frasca <mario at anche dot no> > + > +;; Released under the GNU General Public License version 3 > +;; see: http://www.gnu.org/licenses/gpl-3.0.html Could you update this header to match the style used by other tests (see, e.g., test-org-num.el)? Tests themselves look good to me. Thanks for adding them. I think it'd also be good to add a test for the with-longer-than-deps case I mentioned above. The manual should also be updated to mention this new feature, and it'd be good to have an ORG-NEWS entry. Thanks for working on this.