Re: [PATCH] org-plot abstractions and extension

[Kyle Meyer <kyle@kyleam.com>]

TEC writes:

> Kyle Meyer <kyle@kyleam.com> 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 &optional 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.