Re: [PATCH] org-plot abstractions and extension

[TEC]

Kyle Meyer <kyle@kyleam.com> writes:

> 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.

This makes sense.

> cl-case isn't in cl-lib, and there is no need to load anything.

Huh, interesting.

> recent org-plot example from 8d5122fc5:
> [...]
> That could be rewritten as [...]

Would you like me to bundle that change in somewhere?

>>>> @@ -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'

Ooops, I thought you were referring to one of the other regions (I saw
the "let" and "mapcar" and my brain pattern-matched the rest :P)

One question, I saw Bastien say that we didn't want whitespace-only
commits, so how should whitespace-fixups be done?

>> 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.

Ah, forgot I needed that. Sorted :)

(final?) patch revision attached.

--
Timothy

0001-org-plot.el-fix-compiler-warnings.patch
Description: Text Data