Re: `with` as a list.

[Kyle Meyer <kyle@kyleam.com>]

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.