[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH] org-plot abstractions and extension
From: |
Kyle Meyer |
Subject: |
Re: [PATCH] org-plot abstractions and extension |
Date: |
Wed, 23 Dec 2020 16:55:50 GMT |
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.
- Re: [PATCH] org-plot abstractions and extension, TEC, 2020/12/08
- Re: [PATCH] org-plot abstractions and extension, Bastien, 2020/12/14
- Re: [PATCH] org-plot abstractions and extension, TEC, 2020/12/14
- Re: [PATCH] org-plot abstractions and extension, Kyle Meyer, 2020/12/23
- Re: [PATCH] org-plot abstractions and extension, TEC, 2020/12/23
- Re: [PATCH] org-plot abstractions and extension, TEC, 2020/12/23
- Re: [PATCH] org-plot abstractions and extension, Kyle Meyer, 2020/12/23
- Re: [PATCH] org-plot abstractions and extension, TEC, 2020/12/23
- Re: [PATCH] org-plot abstractions and extension,
Kyle Meyer <=
- Re: [PATCH] org-plot abstractions and extension, TEC, 2020/12/23
- Re: [PATCH] org-plot abstractions and extension, Kyle Meyer, 2020/12/23