From mboxrd@z Thu Jan 1 00:00:00 1970 From: John Kitchin Subject: Re: [PATCH] Make lexical eval default for elisp src blocks Date: Mon, 18 Apr 2016 13:09:41 -0400 Message-ID: References: <1460855140-36680-1-git-send-email-jkitchin@andrew.cmu.edu> <87inzeeuss.fsf@nicolasgoaziou.fr> Mime-Version: 1.0 Content-Type: multipart/alternative; boundary=001a11469b982a5c210530c56dc3 Return-path: Received: from eggs.gnu.org ([2001:4830:134:3::10]:58426) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1asCgC-0007Rx-Cl for emacs-orgmode@gnu.org; Mon, 18 Apr 2016 13:09:45 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1asCgA-0004DU-Lh for emacs-orgmode@gnu.org; Mon, 18 Apr 2016 13:09:44 -0400 Received: from mail-wm0-x231.google.com ([2a00:1450:400c:c09::231]:36353) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1asCgA-0004DM-Ag for emacs-orgmode@gnu.org; Mon, 18 Apr 2016 13:09:42 -0400 Received: by mail-wm0-x231.google.com with SMTP id v188so129087526wme.1 for ; Mon, 18 Apr 2016 10:09:42 -0700 (PDT) In-Reply-To: <87inzeeuss.fsf@nicolasgoaziou.fr> List-Id: "General discussions about Org-mode." List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: emacs-orgmode-bounces+geo-emacs-orgmode=m.gmane.org@gnu.org Sender: "Emacs-orgmode" To: John Kitchin , "emacs-orgmode@gnu.org" --001a11469b982a5c210530c56dc3 Content-Type: text/plain; charset=UTF-8 Thanks for the feedback. I have a few questions below. On Mon, Apr 18, 2016 at 12:38 PM, Nicolas Goaziou wrote: > Hello, > > John Kitchin writes: > > > Set default in `org-babel-default-header-args:emacs-lisp'. Add an > > optional argument to the eval function. > > Thanks for the patch. Some comments follow. > > > +*** Default lexical evaluation of emacs-lisp src blocks > > +Emacs-lisp src blocks in babel are now evaluated using lexical scoping. > There is a new header to control this behavior. > > + > > +The default results in an eval with lexical scoping. > > +:lexical yes > > + > > +This turns lexical scoping off in the eval (the former behavior). > > +:lexical no > > + > > +This uses the lexical environment with x=42 in the eval. > > +:lexical '((x . 42)) > > According to the Elisp manual: > > The value of LEXICAL can also be a non-empty alist specifying > a particular "lexical environment" for lexical bindings; however, > this feature is only useful for specialized purposes, such as in > Emacs Lisp debuggers. *Note Lexical Binding::. > > I'm not opposed to it, but is there any reason to include the > opportunity to specify an alist here? > I just put it in because it is an option for the eval function, and it was not difficult to implement. It might be useful for debugging. > > > +(defvar org-babel-default-header-args:emacs-lisp > > + '((:lexical . "yes")) > > + "Default arguments for evaluating an emacs-lisp source block. > > + > > +:lexical is \"yes\" by default and causes src blocks to be eval'd > > +using lexical scoping. It can also be an alist mapping symbols to > > +their value. It is used as the optional LEXICAL argument to > > +`eval'.") > > You need to separate sentences with two spaces. > no problem. > > You also need to add (defconst org-babel-header-args:emacs-lisp). See > for example `org-babel-header-args:R'. > Are you suggesting use defconst instead of defvar? Does it really need all the things in org-babel-header-args:R? Or just the required default for lexical? > > > (defun org-babel-expand-body:emacs-lisp (body params) > > "Expand BODY according to PARAMS, return the expanded body." > > @@ -51,13 +57,18 @@ > > (defun org-babel-execute:emacs-lisp (body params) > > "Execute a block of emacs-lisp code with Babel." > > (save-window-excursion > > - (let ((result > > - (eval (read (format (if (member "output" > > - (cdr (assoc :result-params > params))) > > - "(with-output-to-string %s)" > > - "(progn %s)") > > - (org-babel-expand-body:emacs-lisp > > - body params)))))) > > + (let* ((lexical (cdr (assoc :lexical params))) > > Nitpick: (cdr (assq :lexical params)) > no problem. > > > + (result > > + (eval (read (format (if (member "output" > > + (cdr (assoc :result-params > params))) > > Nitpick, while you're at it: (cdr (assq :result-params params)) > no problem. Should all the assocs be replaced by assq, or just these ones? > > > + "(with-output-to-string %s)" > > + "(progn %s)") > > + (org-babel-expand-body:emacs-lisp > > + body params))) > > + > > + (if (listp lexical) > > + lexical > > + (string= "yes" lexical))))) > > There is no support for t. I think we should allow both :lexical > t and :lexical yes. > something like (member lexical '("yes" "t"))? > > > Regards, > > -- > Nicolas Goaziou > --001a11469b982a5c210530c56dc3 Content-Type: text/html; charset=UTF-8 Content-Transfer-Encoding: quoted-printable
Thanks for the feedback. I have a few questions below.
=


On Mon, Apr 18, 2016 at 12:38 PM, Nicolas Goaziou <mail@nicolasgoaziou.fr> wrote:
Hello,
John Kitchin <jkitchin@andrew= .cmu.edu> writes:

> Set default in `org-babel-default-header-args:emacs-lisp'. Add an<= br> > optional argument to the eval function.

Thanks for the patch. Some comments follow.

> +*** Default lexical evaluation of emacs-lisp src blocks
> +Emacs-lisp src blocks in babel are now evaluated using lexical scopin= g. There is a new header to control this behavior.
> +
> +The default results in an eval with lexical scoping.
> +:lexical yes
> +
> +This turns lexical scoping off in the eval (the former behavior).
> +:lexical no
> +
> +This uses the lexical environment with x=3D42 in the eval.
> +:lexical '((x . 42))

According to the Elisp manual:

=C2=A0 =C2=A0 =C2=A0The value of LEXICAL can also be a non-empty alist spec= ifying
=C2=A0 =C2=A0 =C2=A0a particular "lexical environment" for lexica= l bindings; however,
=C2=A0 =C2=A0 =C2=A0this feature is only useful for specialized purposes, s= uch as in
=C2=A0 =C2=A0 =C2=A0Emacs Lisp debuggers. *Note Lexical Binding::.

I'm not opposed to it, but is there any reason to include the
opportunity to specify an alist here?

I= just put it in because it is an option for the eval function, and it was n= ot difficult to implement. It might be useful for debugging.
=C2= =A0

> +(defvar org-babel-default-header-args:emacs-lisp
> +=C2=A0 '((:lexical . "yes"))
> +=C2=A0 "Default arguments for evaluating an emacs-lisp source bl= ock.
> +
> +:lexical is \"yes\" by default and causes src blocks to be = eval'd
> +using lexical scoping. It can also be an alist mapping symbols to
> +their value. It is used as the optional LEXICAL argument to
> +`eval'.")

You need to separate sentences with two spaces.

no problem.
=C2=A0

You also need to add (defconst org-babel-header-args:emacs-lisp). See
for example `org-babel-header-args:R'.

<= div>Are you suggesting use defconst instead of defvar? Does it really need = all the things in org-babel-header-args:R? Or just the required default for= lexical?
=C2=A0

>=C2=A0 (defun org-babel-expand-body:emacs-lisp (body params)
>=C2=A0 =C2=A0 "Expand BODY according to PARAMS, return the expande= d body."
> @@ -51,13 +57,18 @@
>=C2=A0 (defun org-babel-execute:emacs-lisp (body params)
>=C2=A0 =C2=A0 "Execute a block of emacs-lisp code with Babel."= ;
>=C2=A0 =C2=A0 (save-window-excursion
> -=C2=A0 =C2=A0 (let ((result
> -=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0(eval (read (format (if (mem= ber "output"
> -=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0(cdr (assoc :result-params params)))
> -=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0"(with-output-= to-string %s)"
> -=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0"(progn %s)") > -=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0(org-babel-expand-body:emacs-lisp=
> -=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 body params))))))
> +=C2=A0 =C2=A0 (let* ((lexical (cdr (assoc :lexical params)))

Nitpick: (cdr (assq :lexical params))

no problem.=C2=A0
=C2=A0

> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 (result
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0(eval (read (format (if (member &qu= ot;output"
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0(cdr (assoc :result-params params)))

Nitpick, while you're at it: (cdr (assq :result-params params))<= br>

no problem. Should all the assocs be re= placed by assq, or just these ones?

> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0"(with-output-to-stri= ng %s)"
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0"(progn %s)")
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0(org-babel-expand-body:emacs-lisp
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 body params)))
> +
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0(if (listp lex= ical)
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= lexical
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0(string= =3D "yes" lexical)))))

There is no support for t. I think we should allow both :lexical
t and :lexical yes.

something like (mem= ber lexical '("yes" "t"))?
=C2=A0


Regards,

--
Nicolas Goaziou

--001a11469b982a5c210530c56dc3--