emacs-orgmode@gnu.org archives
 help / color / mirror / code / Atom feed
From: Kyle Meyer <kyle@kyleam.com>
To: TEC <tecosaur@gmail.com>
Cc: Bastien <bzg@gnu.org>, org-mode-email <emacs-orgmode@gnu.org>
Subject: Re: [PATCH] org-plot abstractions and extension
Date: Wed, 23 Dec 2020 16:55:50 GMT	[thread overview]
Message-ID: <871rfgh36z.fsf@kyleam.com> (raw)
In-Reply-To: <87wnx8ud5k.fsf@gmail.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.


  reply	other threads:[~2020-12-23 16:56 UTC|newest]

Thread overview: 20+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-09-06  5:50 TEC
     [not found] ` <87blijmnv9.fsf@gnu.org>
     [not found]   ` <CAHNg_jM8sE4a6XvL5D8Gks4dQXfWhZvRBR33BDLkRgEgZ++ZGg@mail.gmail.com>
2020-09-15  3:43     ` TEC
2020-09-25 17:51       ` TEC
2020-10-17  2:12         ` TEC
2020-10-24 11:31           ` Bastien
2020-10-24 18:16             ` TEC
2020-11-21 11:49               ` ian martins
2020-12-09  2:58               ` TEC
2020-12-10 10:28                 ` Bastien
2020-12-14  5:41               ` Bastien
2020-12-14  6:30                 ` TEC
2020-12-14  6:56                   ` Bastien
2020-12-23  5:09                 ` Kyle Meyer
2020-12-23  5:10                   ` TEC
2020-12-23  6:19                     ` TEC
2020-12-23  7:14                       ` Kyle Meyer
2020-12-23  8:38                         ` TEC
2020-12-23 16:55                           ` Kyle Meyer [this message]
2020-12-23 18:19                             ` TEC
2020-12-24  3:09                               ` Kyle Meyer

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: https://www.orgmode.org/

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=871rfgh36z.fsf@kyleam.com \
    --to=kyle@kyleam.com \
    --cc=bzg@gnu.org \
    --cc=emacs-orgmode@gnu.org \
    --cc=tecosaur@gmail.com \
    --subject='Re: [PATCH] org-plot abstractions and extension' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Code repositories for project(s) associated with this inbox:

	https://git.savannah.gnu.org/cgit/emacs/org-mode.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).