Re: `with` as a list.

[Kyle Meyer]

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.