* ob-python newline & indentation behavior
@ 2017-11-18 12:02 Jack Kamm
2017-11-18 15:05 ` Kyle Meyer
0 siblings, 1 reply; 19+ messages in thread
From: Jack Kamm @ 2017-11-18 12:02 UTC (permalink / raw)
To: emacs-orgmode
ob-python newline & indentation behavior in :session is very ugly and
possibly broken. For example, consider the following code block:
#+BEGIN_SRC python :session :results output
foo = 0
for _ in range(10):
foo += 1
foo += 1
print(foo)
#+END_SRC
Ideally this would print "20", but instead it prints "11",
because the second "foo+=1" is executed after the for loop.
OTOH "python-shell-send-region" (from python.el) gives the correct
answer of "20".
The inconsistent behavior of "python-shell-send-region" and
"org-babel-eval" often causes me problems, because I want to use both
(the former for testing and async eval; the latter for inserting into
the document).
There is a 2 year old patch that fixes this behavior but has not yet
been incorporated:
https://lists.gnu.org/archive/html/emacs-orgmode/2015-03/msg00505.html
The patch follows the python.el behavior of using a temporary file and
executing that from the shell.
Could this patch, or something similar, be incorporated into org-mode,
to fix this behavior?
Thanks,
Jack Kamm
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: ob-python newline & indentation behavior 2017-11-18 12:02 ob-python newline & indentation behavior Jack Kamm @ 2017-11-18 15:05 ` Kyle Meyer 2017-11-18 22:15 ` Jack Kamm 0 siblings, 1 reply; 19+ messages in thread From: Kyle Meyer @ 2017-11-18 15:05 UTC (permalink / raw) To: Jack Kamm, emacs-orgmode Hello, Jack Kamm <jackkamm@gmail.com> writes: > ob-python newline & indentation behavior in :session is very ugly and > possibly broken. For example, consider the following code block: [...] > There is a 2 year old patch that fixes this behavior but has not yet > been incorporated: > https://lists.gnu.org/archive/html/emacs-orgmode/2015-03/msg00505.html [...] > The patch follows the python.el behavior of using a temporary file and > executing that from the shell. > > Could this patch, or something similar, be incorporated into org-mode, > to fix this behavior? Here's what I said in a recent post about ob-python sessions: https://lists.gnu.org/archive/html/emacs-orgmode/2017-10/msg00198.html I dropped my attempt to fix it because 1) I was still having trouble getting a complete understanding of what the issue was and 2) I didn't have the motivation to spend time digging deeper because I don't use ob-python (and in general am not a heavy Org-Babel user). Perhaps you or some other ob-python user could help make ob-python sessions more robust? Perhaps you are the "you"? -- Kyle ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: ob-python newline & indentation behavior 2017-11-18 15:05 ` Kyle Meyer @ 2017-11-18 22:15 ` Jack Kamm 2017-11-19 3:27 ` Martin Alsinet 2017-11-19 17:17 ` Kyle Meyer 0 siblings, 2 replies; 19+ messages in thread From: Jack Kamm @ 2017-11-18 22:15 UTC (permalink / raw) To: Kyle Meyer; +Cc: emacs-orgmode [-- Attachment #1: Type: text/plain, Size: 3889 bytes --] Thanks Kyle, and sorry for missing that recent related thread. I adapted your old patch to the current master branch. I also extended it to work for ":results value" (the original patch only worked for ":results output"). I did this by not writing the last line of the code block to the tmpfile, unless it is indented. I've never contributed before so would appreciate any comments on this patch, and how to get it accepted. Cheers, Jack. From a5d553ece9f6ee35cd1e273e554a21a19e80ec3c Mon Sep 17 00:00:00 2001 From: Jack Kamm <jackkamm@gmail.com> Date: Sat, 18 Nov 2017 21:47:09 +0000 Subject: [PATCH] fix newline/indentation issues in ob-python :session --- lisp/ob-python.el | 33 +++++++++++++++++++++++++++++++++ 1 file changed, 33 insertions(+) diff --git a/lisp/ob-python.el b/lisp/ob-python.el index 60ec5fa47..6623ef5fc 100644 --- a/lisp/ob-python.el +++ b/lisp/ob-python.el @@ -303,6 +303,10 @@ last statement in BODY, as elisp." (mapc (lambda (line) (insert line) (funcall send-wait)) (split-string body "[\r\n]")) (funcall send-wait))) + (indented-p (org-babel-python--indented-p body)) + (body (if indented-p + (org-babel-python--replace-body-tmpfile body) + body)) (results (pcase result-type (`output @@ -340,6 +344,35 @@ last statement in BODY, as elisp." (substring string 1 -1) string)) + +(defun org-babel-python--indented-p (input) + "Non-nil if any line in INPUT is indented." + (string-match-p "^[ \t]" input)) + +(defun org-babel-python--replace-body-tmpfile (body) + "Place body in tmpfile, and return string to exec the tmpfile. +If last line of body is not indented, place it at end of exec string +instead of tmpfile, so shell can see the result" + (let* ((tmp-file (org-babel-temp-file "python-")) + (lines (split-string body "\n" t)) + (lastline (car (last lines))) + (newbody (concat + (format "__pyfilename = '%s'; " tmp-file) + "__pyfile = open(__pyfilename); " + "exec(compile(" + "__pyfile.read(), __pyfilename, 'exec'" + ")); " + "__pyfile.close()"))) + (if (string-match-p "^[ \t]" lastline) + (progn + (with-temp-file tmp-file (insert body)) + newbody) + (with-temp-file tmp-file + (insert (mapconcat 'identity + (butlast lines) "\n"))) + (concat newbody "\n" lastline))) + ) + (provide 'ob-python) -- 2.15.0 On Sat, Nov 18, 2017 at 3:05 PM, Kyle Meyer <kyle@kyleam.com> wrote: > Hello, > > Jack Kamm <jackkamm@gmail.com> writes: > > > ob-python newline & indentation behavior in :session is very ugly and > > possibly broken. For example, consider the following code block: > > [...] > > > There is a 2 year old patch that fixes this behavior but has not yet > > been incorporated: > > https://lists.gnu.org/archive/html/emacs-orgmode/2015-03/msg00505.html > > [...] > > > The patch follows the python.el behavior of using a temporary file and > > executing that from the shell. > > > > Could this patch, or something similar, be incorporated into org-mode, > > to fix this behavior? > > Here's what I said in a recent post about ob-python sessions: > > https://lists.gnu.org/archive/html/emacs-orgmode/2017-10/msg00198.html > > I dropped my attempt to fix it because 1) I was still having trouble > getting a complete understanding of what the issue was and 2) I didn't > have the motivation to spend time digging deeper because I don't use > ob-python (and in general am not a heavy Org-Babel user). Perhaps you > or some other ob-python user could help make ob-python sessions more > robust? > > Perhaps you are the "you"? > > -- > Kyle > [-- Attachment #2: Type: text/html, Size: 6041 bytes --] ^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: ob-python newline & indentation behavior 2017-11-18 22:15 ` Jack Kamm @ 2017-11-19 3:27 ` Martin Alsinet 2017-11-19 3:34 ` Martin Alsinet 2017-11-19 17:17 ` Kyle Meyer 1 sibling, 1 reply; 19+ messages in thread From: Martin Alsinet @ 2017-11-19 3:27 UTC (permalink / raw) To: Jack Kamm; +Cc: emacs-orgmode [-- Attachment #1: Type: text/plain, Size: 4411 bytes --] Hello Jack: What versions of emacs and org-mode are you using? I tried your example on Emacs 25.3.1 and org-mode 9.1.2-22 and I got "20" as result It has happened to me in the past that some bug I was seeing goes away just by updating org-mode. Regards, Martin On Sat, Nov 18, 2017 at 5:16 PM Jack Kamm <jackkamm@gmail.com> wrote: > Thanks Kyle, and sorry for missing that recent related thread. > > I adapted your old patch to the current master branch. I also extended it > to work for ":results value" (the original patch only worked for ":results > output"). I did this by not writing the last line of the code block to the > tmpfile, unless it is indented. > > I've never contributed before so would appreciate any comments on this > patch, and how to get it accepted. Cheers, Jack. > > From a5d553ece9f6ee35cd1e273e554a21a19e80ec3c Mon Sep 17 00:00:00 2001 > > From: Jack Kamm <jackkamm@gmail.com> > Date: Sat, 18 Nov 2017 21:47:09 +0000 > Subject: [PATCH] fix newline/indentation issues in ob-python :session > > --- > lisp/ob-python.el | 33 +++++++++++++++++++++++++++++++++ > 1 file changed, 33 insertions(+) > > diff --git a/lisp/ob-python.el b/lisp/ob-python.el > index 60ec5fa47..6623ef5fc 100644 > --- a/lisp/ob-python.el > +++ b/lisp/ob-python.el > @@ -303,6 +303,10 @@ last statement in BODY, as elisp." > (mapc (lambda (line) (insert line) (funcall > send-wait)) > (split-string body "[\r\n]")) > (funcall send-wait))) > + (indented-p (org-babel-python--indented-p body)) > + (body (if indented-p > + (org-babel-python--replace-body-tmpfile body) > + body)) > (results > (pcase result-type > (`output > @@ -340,6 +344,35 @@ last statement in BODY, as elisp." > (substring string 1 -1) > string)) > > + > +(defun org-babel-python--indented-p (input) > + "Non-nil if any line in INPUT is indented." > + (string-match-p "^[ \t]" input)) > + > +(defun org-babel-python--replace-body-tmpfile (body) > + "Place body in tmpfile, and return string to exec the tmpfile. > +If last line of body is not indented, place it at end of exec string > +instead of tmpfile, so shell can see the result" > + (let* ((tmp-file (org-babel-temp-file "python-")) > + (lines (split-string body "\n" t)) > + (lastline (car (last lines))) > + (newbody (concat > + (format "__pyfilename = '%s'; " tmp-file) > + "__pyfile = open(__pyfilename); " > + "exec(compile(" > + "__pyfile.read(), __pyfilename, 'exec'" > + ")); " > + "__pyfile.close()"))) > + (if (string-match-p "^[ \t]" lastline) > + (progn > + (with-temp-file tmp-file (insert body)) > + newbody) > + (with-temp-file tmp-file > + (insert (mapconcat 'identity > + (butlast lines) "\n"))) > + (concat newbody "\n" lastline))) > + ) > + > (provide 'ob-python) > > > -- > 2.15.0 > > > On Sat, Nov 18, 2017 at 3:05 PM, Kyle Meyer <kyle@kyleam.com> wrote: > >> Hello, >> >> Jack Kamm <jackkamm@gmail.com> writes: >> >> > ob-python newline & indentation behavior in :session is very ugly and >> > possibly broken. For example, consider the following code block: >> >> [...] >> >> > There is a 2 year old patch that fixes this behavior but has not yet >> > been incorporated: >> > https://lists.gnu.org/archive/html/emacs-orgmode/2015-03/msg00505.html >> >> [...] >> >> > The patch follows the python.el behavior of using a temporary file and >> > executing that from the shell. >> > >> > Could this patch, or something similar, be incorporated into org-mode, >> > to fix this behavior? >> >> Here's what I said in a recent post about ob-python sessions: >> >> >> https://lists.gnu.org/archive/html/emacs-orgmode/2017-10/msg00198.html >> >> I dropped my attempt to fix it because 1) I was still having trouble >> getting a complete understanding of what the issue was and 2) I didn't >> have the motivation to spend time digging deeper because I don't use >> ob-python (and in general am not a heavy Org-Babel user). Perhaps you >> or some other ob-python user could help make ob-python sessions more >> robust? >> >> Perhaps you are the "you"? >> >> -- >> Kyle >> > > [-- Attachment #2: Type: text/html, Size: 6783 bytes --] ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: ob-python newline & indentation behavior 2017-11-19 3:27 ` Martin Alsinet @ 2017-11-19 3:34 ` Martin Alsinet 2017-11-19 7:41 ` Jack Kamm 0 siblings, 1 reply; 19+ messages in thread From: Martin Alsinet @ 2017-11-19 3:34 UTC (permalink / raw) To: Jack Kamm; +Cc: emacs-orgmode [-- Attachment #1: Type: text/plain, Size: 4713 bytes --] Sorry Jack, I overlooked the :session bit. Disregard my email please On Sat, Nov 18, 2017 at 10:27 PM Martin Alsinet <martin@alsinet.com.ar> wrote: > Hello Jack: > > What versions of emacs and org-mode are you using? > > I tried your example on Emacs 25.3.1 and org-mode 9.1.2-22 and I got "20" > as result > It has happened to me in the past that some bug I was seeing goes away > just by updating org-mode. > > Regards, > > > Martin > > On Sat, Nov 18, 2017 at 5:16 PM Jack Kamm <jackkamm@gmail.com> wrote: > >> Thanks Kyle, and sorry for missing that recent related thread. >> >> I adapted your old patch to the current master branch. I also extended it >> to work for ":results value" (the original patch only worked for ":results >> output"). I did this by not writing the last line of the code block to the >> tmpfile, unless it is indented. >> >> I've never contributed before so would appreciate any comments on this >> patch, and how to get it accepted. Cheers, Jack. >> >> From a5d553ece9f6ee35cd1e273e554a21a19e80ec3c Mon Sep 17 00:00:00 2001 >> >> From: Jack Kamm <jackkamm@gmail.com> >> Date: Sat, 18 Nov 2017 21:47:09 +0000 >> Subject: [PATCH] fix newline/indentation issues in ob-python :session >> >> --- >> lisp/ob-python.el | 33 +++++++++++++++++++++++++++++++++ >> 1 file changed, 33 insertions(+) >> >> diff --git a/lisp/ob-python.el b/lisp/ob-python.el >> index 60ec5fa47..6623ef5fc 100644 >> --- a/lisp/ob-python.el >> +++ b/lisp/ob-python.el >> @@ -303,6 +303,10 @@ last statement in BODY, as elisp." >> (mapc (lambda (line) (insert line) (funcall >> send-wait)) >> (split-string body "[\r\n]")) >> (funcall send-wait))) >> + (indented-p (org-babel-python--indented-p body)) >> + (body (if indented-p >> + (org-babel-python--replace-body-tmpfile body) >> + body)) >> (results >> (pcase result-type >> (`output >> @@ -340,6 +344,35 @@ last statement in BODY, as elisp." >> (substring string 1 -1) >> string)) >> >> + >> +(defun org-babel-python--indented-p (input) >> + "Non-nil if any line in INPUT is indented." >> + (string-match-p "^[ \t]" input)) >> + >> +(defun org-babel-python--replace-body-tmpfile (body) >> + "Place body in tmpfile, and return string to exec the tmpfile. >> +If last line of body is not indented, place it at end of exec string >> +instead of tmpfile, so shell can see the result" >> + (let* ((tmp-file (org-babel-temp-file "python-")) >> + (lines (split-string body "\n" t)) >> + (lastline (car (last lines))) >> + (newbody (concat >> + (format "__pyfilename = '%s'; " tmp-file) >> + "__pyfile = open(__pyfilename); " >> + "exec(compile(" >> + "__pyfile.read(), __pyfilename, 'exec'" >> + ")); " >> + "__pyfile.close()"))) >> + (if (string-match-p "^[ \t]" lastline) >> + (progn >> + (with-temp-file tmp-file (insert body)) >> + newbody) >> + (with-temp-file tmp-file >> + (insert (mapconcat 'identity >> + (butlast lines) "\n"))) >> + (concat newbody "\n" lastline))) >> + ) >> + >> (provide 'ob-python) >> >> >> -- >> 2.15.0 >> >> >> On Sat, Nov 18, 2017 at 3:05 PM, Kyle Meyer <kyle@kyleam.com> wrote: >> >>> Hello, >>> >>> Jack Kamm <jackkamm@gmail.com> writes: >>> >>> > ob-python newline & indentation behavior in :session is very ugly and >>> > possibly broken. For example, consider the following code block: >>> >>> [...] >>> >>> > There is a 2 year old patch that fixes this behavior but has not yet >>> > been incorporated: >>> > https://lists.gnu.org/archive/html/emacs-orgmode/2015-03/msg00505.html >>> >>> [...] >>> >>> > The patch follows the python.el behavior of using a temporary file and >>> > executing that from the shell. >>> > >>> > Could this patch, or something similar, be incorporated into org-mode, >>> > to fix this behavior? >>> >>> Here's what I said in a recent post about ob-python sessions: >>> >>> >>> https://lists.gnu.org/archive/html/emacs-orgmode/2017-10/msg00198.html >>> >>> I dropped my attempt to fix it because 1) I was still having trouble >>> getting a complete understanding of what the issue was and 2) I >>> didn't >>> have the motivation to spend time digging deeper because I don't use >>> ob-python (and in general am not a heavy Org-Babel user). Perhaps >>> you >>> or some other ob-python user could help make ob-python sessions more >>> robust? >>> >>> Perhaps you are the "you"? >>> >>> -- >>> Kyle >>> >> >> [-- Attachment #2: Type: text/html, Size: 7236 bytes --] ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: ob-python newline & indentation behavior 2017-11-19 3:34 ` Martin Alsinet @ 2017-11-19 7:41 ` Jack Kamm 2017-11-20 6:25 ` Kyle Meyer 0 siblings, 1 reply; 19+ messages in thread From: Jack Kamm @ 2017-11-19 7:41 UTC (permalink / raw) To: Martin Alsinet; +Cc: emacs-orgmode [-- Attachment #1: Type: text/plain, Size: 8459 bytes --] Sorry, I should have mentioned my version info anyways. I have tested on emacs 25.3.1 and emacs 26.0.90, and org-mode versions 9.1.2 and 9.1.3 (current master). The same error occurs on all emacs and org-mode versions. However the error slightly differs between Python and IPython interpreters. IPython prints "11" (instead of the expected "20"), whereas Python raises an IndentationError then prints "10". I've cleaned up my patch to deal with a few edge cases. In particular: 1) Fixed a bug where I was removing blank lines (these may be needed if they are part of a multiline string object) 2) Send to tmpfile any multiline block even if not indented. This follows the python.el behavior and gives more consistent behavior for ":results output" 3) Allow ":results value" to work when the block ends in several newlines, using string-trim-right Here is the new version of the patch: From f009da37d3b7e2730abb8cbb10f4d07b3d456dd8 Mon Sep 17 00:00:00 2001 From: Jack Kamm <jackkamm@gmail.com> Date: Sun, 19 Nov 2017 07:13:56 +0000 Subject: [PATCH] Squashed commit of the following: commit d1fe88a9f61a8e7082f08b7c190a29737bb655d5 Author: Jack Kamm <jackkamm@gmail.com> Date: Sun Nov 19 07:08:31 2017 +0000 fix block ending in blank lines; send multiline blocks to tmpfile commit fcc5a7795e882716775c9d925b0cd5b657da041b Author: Jack Kamm <jackkamm@gmail.com> Date: Sat Nov 18 22:40:31 2017 +0000 fix newlines and blanklines when sending codeblock to tmpfile commit a5d553ece9f6ee35cd1e273e554a21a19e80ec3c Author: Jack Kamm <jackkamm@gmail.com> Date: Sat Nov 18 21:47:09 2017 +0000 fix newline/indentation issues in ob-python :session --- lisp/ob-python.el | 29 +++++++++++++++++++++++++++++ 1 file changed, 29 insertions(+) diff --git a/lisp/ob-python.el b/lisp/ob-python.el index 60ec5fa47..c3dba1565 100644 --- a/lisp/ob-python.el +++ b/lisp/ob-python.el @@ -303,6 +303,9 @@ last statement in BODY, as elisp." (mapc (lambda (line) (insert line) (funcall send-wait)) (split-string body "[\r\n]")) (funcall send-wait))) + (body (if (string-match-p ".\n+." body) ;; Multiline + (org-babel-python--replace-body-tmpfile body) + body)) (results (pcase result-type (`output @@ -340,6 +343,32 @@ last statement in BODY, as elisp." (substring string 1 -1) string)) + +(defun org-babel-python--replace-body-tmpfile (body) + "Place body in tmpfile, and return string to exec the tmpfile. +If last line of body is not indented, place it at end of exec string +instead of tmpfile, so shell can see the result" + (let* ((body (string-trim-right body)) + (tmp-file (org-babel-temp-file "python-")) + (lines (split-string body "[\r\n]")) + (lastline (car (last lines))) + (newbody (concat + (format "__pyfilename = '%s'; " tmp-file) + "__pyfile = open(__pyfilename); " + "exec(compile(" + "__pyfile.read(), __pyfilename, 'exec'" + ")); " + "__pyfile.close()"))) + (if (string-match-p "^[ \t]" lastline) + (progn + (with-temp-file tmp-file (insert body)) + newbody) + (with-temp-file tmp-file + (insert (mapconcat 'identity + (butlast lines) "\n"))) + (concat newbody "\n" lastline))) + ) + (provide 'ob-python) -- 2.15.0 On Sun, Nov 19, 2017 at 3:34 AM, Martin Alsinet <martin@alsinet.com.ar> wrote: > Sorry Jack, I overlooked the :session bit. > Disregard my email please > > > On Sat, Nov 18, 2017 at 10:27 PM Martin Alsinet <martin@alsinet.com.ar> > wrote: > >> Hello Jack: >> >> What versions of emacs and org-mode are you using? >> >> I tried your example on Emacs 25.3.1 and org-mode 9.1.2-22 and I got "20" >> as result >> It has happened to me in the past that some bug I was seeing goes away >> just by updating org-mode. >> >> Regards, >> >> >> Martin >> >> On Sat, Nov 18, 2017 at 5:16 PM Jack Kamm <jackkamm@gmail.com> wrote: >> >>> Thanks Kyle, and sorry for missing that recent related thread. >>> >>> I adapted your old patch to the current master branch. I also extended >>> it to work for ":results value" (the original patch only worked for >>> ":results output"). I did this by not writing the last line of the code >>> block to the tmpfile, unless it is indented. >>> >>> I've never contributed before so would appreciate any comments on this >>> patch, and how to get it accepted. Cheers, Jack. >>> >>> From a5d553ece9f6ee35cd1e273e554a21a19e80ec3c Mon Sep 17 00:00:00 2001 >>> >>> From: Jack Kamm <jackkamm@gmail.com> >>> Date: Sat, 18 Nov 2017 21:47:09 +0000 >>> Subject: [PATCH] fix newline/indentation issues in ob-python :session >>> >>> --- >>> lisp/ob-python.el | 33 +++++++++++++++++++++++++++++++++ >>> 1 file changed, 33 insertions(+) >>> >>> diff --git a/lisp/ob-python.el b/lisp/ob-python.el >>> index 60ec5fa47..6623ef5fc 100644 >>> --- a/lisp/ob-python.el >>> +++ b/lisp/ob-python.el >>> @@ -303,6 +303,10 @@ last statement in BODY, as elisp." >>> (mapc (lambda (line) (insert line) (funcall >>> send-wait)) >>> (split-string body "[\r\n]")) >>> (funcall send-wait))) >>> + (indented-p (org-babel-python--indented-p body)) >>> + (body (if indented-p >>> + (org-babel-python--replace-body-tmpfile body) >>> + body)) >>> (results >>> (pcase result-type >>> (`output >>> @@ -340,6 +344,35 @@ last statement in BODY, as elisp." >>> (substring string 1 -1) >>> string)) >>> >>> + >>> +(defun org-babel-python--indented-p (input) >>> + "Non-nil if any line in INPUT is indented." >>> + (string-match-p "^[ \t]" input)) >>> + >>> +(defun org-babel-python--replace-body-tmpfile (body) >>> + "Place body in tmpfile, and return string to exec the tmpfile. >>> +If last line of body is not indented, place it at end of exec string >>> +instead of tmpfile, so shell can see the result" >>> + (let* ((tmp-file (org-babel-temp-file "python-")) >>> + (lines (split-string body "\n" t)) >>> + (lastline (car (last lines))) >>> + (newbody (concat >>> + (format "__pyfilename = '%s'; " tmp-file) >>> + "__pyfile = open(__pyfilename); " >>> + "exec(compile(" >>> + "__pyfile.read(), __pyfilename, 'exec'" >>> + ")); " >>> + "__pyfile.close()"))) >>> + (if (string-match-p "^[ \t]" lastline) >>> + (progn >>> + (with-temp-file tmp-file (insert body)) >>> + newbody) >>> + (with-temp-file tmp-file >>> + (insert (mapconcat 'identity >>> + (butlast lines) "\n"))) >>> + (concat newbody "\n" lastline))) >>> + ) >>> + >>> (provide 'ob-python) >>> >>> >>> -- >>> 2.15.0 >>> >>> >>> On Sat, Nov 18, 2017 at 3:05 PM, Kyle Meyer <kyle@kyleam.com> wrote: >>> >>>> Hello, >>>> >>>> Jack Kamm <jackkamm@gmail.com> writes: >>>> >>>> > ob-python newline & indentation behavior in :session is very ugly and >>>> > possibly broken. For example, consider the following code block: >>>> >>>> [...] >>>> >>>> > There is a 2 year old patch that fixes this behavior but has not yet >>>> > been incorporated: >>>> > https://lists.gnu.org/archive/html/emacs-orgmode/2015-03/ >>>> msg00505.html >>>> >>>> [...] >>>> >>>> > The patch follows the python.el behavior of using a temporary file and >>>> > executing that from the shell. >>>> > >>>> > Could this patch, or something similar, be incorporated into org-mode, >>>> > to fix this behavior? >>>> >>>> Here's what I said in a recent post about ob-python sessions: >>>> >>>> https://lists.gnu.org/archive/html/emacs-orgmode/2017-10/ >>>> msg00198.html >>>> >>>> I dropped my attempt to fix it because 1) I was still having trouble >>>> getting a complete understanding of what the issue was and 2) I >>>> didn't >>>> have the motivation to spend time digging deeper because I don't use >>>> ob-python (and in general am not a heavy Org-Babel user). Perhaps >>>> you >>>> or some other ob-python user could help make ob-python sessions more >>>> robust? >>>> >>>> Perhaps you are the "you"? >>>> >>>> -- >>>> Kyle >>>> >>> >>> [-- Attachment #2: Type: text/html, Size: 12941 bytes --] ^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: ob-python newline & indentation behavior 2017-11-19 7:41 ` Jack Kamm @ 2017-11-20 6:25 ` Kyle Meyer 2017-11-20 21:31 ` Jack Kamm 0 siblings, 1 reply; 19+ messages in thread From: Kyle Meyer @ 2017-11-20 6:25 UTC (permalink / raw) To: Jack Kamm; +Cc: emacs-orgmode Jack Kamm <jackkamm@gmail.com> writes: > Here is the new version of the patch: I haven't had any luck applying this patch to master. Perhaps your email client is altering the inline patch; you can instead attach the output file of 'git format-patch'. > From f009da37d3b7e2730abb8cbb10f4d07b3d456dd8 Mon Sep 17 00:00:00 2001 > > From: Jack Kamm <jackkamm@gmail.com> > Date: Sun, 19 Nov 2017 07:13:56 +0000 > Subject: [PATCH] Squashed commit of the following: > > commit d1fe88a9f61a8e7082f08b7c190a29737bb655d5 > Author: Jack Kamm <jackkamm@gmail.com> > Date: Sun Nov 19 07:08:31 2017 +0000 > > fix block ending in blank lines; send multiline blocks to tmpfile > > commit fcc5a7795e882716775c9d925b0cd5b657da041b > Author: Jack Kamm <jackkamm@gmail.com> > Date: Sat Nov 18 22:40:31 2017 +0000 > > fix newlines and blanklines when sending codeblock to tmpfile > > commit a5d553ece9f6ee35cd1e273e554a21a19e80ec3c > Author: Jack Kamm <jackkamm@gmail.com> > Date: Sat Nov 18 21:47:09 2017 +0000 > > fix newline/indentation issues in ob-python :session Please see <http://orgmode.org/worg/org-contribute.html#commit-messages> for information on Org's convention for commit messages. In addition to following this format, ideally the message would contain a brief description of the problem the patch is fixing. > --- > lisp/ob-python.el | 29 +++++++++++++++++++++++++++++ > 1 file changed, 29 insertions(+) This patch probably passes the TINYCHANGE threshold, so please see <http://orgmode.org/worg/org-contribute.html#copyright-issues> for information about assigning copyright. > diff --git a/lisp/ob-python.el b/lisp/ob-python.el > index 60ec5fa47..c3dba1565 100644 > --- a/lisp/ob-python.el > +++ b/lisp/ob-python.el > @@ -303,6 +303,9 @@ last statement in BODY, as elisp." > (mapc (lambda (line) (insert line) (funcall > send-wait)) > (split-string body "[\r\n]")) > (funcall send-wait))) > + (body (if (string-match-p ".\n+." body) ;; Multiline nitpick: When a comments is on the same line as code, the convention is to use a single ";". I see your point that sending all multiline code through a temporary file is consistent with python.el's behavior. When you say that it "gives more consistent behavior for ":results output", I believe you're talking about problems with ">>>" and "..." markers leaking into the output. I agree that this change should be an improvement since I think most of the prompt issues are racy problems related to sending multiple lines. (I think there still might be a unrelated prompt issue regarding python.el's startup message leaking into the output of the first executed block.) Looking back at my summary of ob-python problems in <https://lists.gnu.org/archive/html/emacs-orgmode/2015-03/msg00540.html>, this seems like a good way to take the solution to problem #3 (indentation, multiline syntax errors) and apply it to problem #1 (markers leaking into the output). Nice, hadn't occurred to me. Assuming we do go this direction, I wonder if there's any ob-python code that can be cleaned up or simplified since the multiline processing logic is no longer needed. > + (org-babel-python--replace-body-tmpfile body) > + body)) > (results > (pcase result-type > (`output > @@ -340,6 +343,32 @@ last statement in BODY, as elisp." > (substring string 1 -1) > string)) > > + > +(defun org-babel-python--replace-body-tmpfile (body) > + "Place body in tmpfile, and return string to exec the tmpfile. nitpick: s/body/BODY/ here and below > +If last line of body is not indented, place it at end of exec string > +instead of tmpfile, so shell can see the result" nitpick: missing trailing period This is the part of the patch that I'm unsure about. I don't like that it * can fail if the last line is a non-indented, continued string * doesn't allow you to get a return value for commons things like conditionals, try-except blocks, context statements. I can't think of a good solution, though. Stepping back a bit, I think it's unfortunate that python blocks handle ":results value" differently depending on whether the block is hooked up to a session or not. For non-sessions, you have to use return. Using the same approach (org-babel-python-wrapper-method) for ":session :results value", we could then get the return value reliably, but the problem with this approach is that any variables defined in a ":results value" block wouldn't be defined in the session after executing the block because the code is wrapped in a function. So at this point I think the choice is between * restricting the return value to unindented last lines and not supporting continued strings in the last line * wrapping the return value with org-babel-python-wrapper-method and not supporting persistent session variables in that block * not supporting ":session :results value" > + (let* ((body (string-trim-right body)) > + (tmp-file (org-babel-temp-file "python-")) > + (lines (split-string body "[\r\n]")) > + (lastline (car (last lines))) > + (newbody (concat > + (format "__pyfilename = '%s'; " tmp-file) Since you're already using concat, I prefer to avoid the format here. Or you could add a defconst that just defines that format string and then only have a format call here. > + "__pyfile = open(__pyfilename); " Right, the leading underscores are good to reduce the risk of clobbering a variable that the user may have defined in the shell. Since this is generated code, I wonder if you should be even more specific and use ugly names like "__org_babel_python_fname" and "__org_babel_python_fh". > + "exec(compile(" > + "__pyfile.read(), __pyfilename, 'exec'" > + ")); " > + "__pyfile.close()"))) > + (if (string-match-p "^[ \t]" lastline) > + (progn > + (with-temp-file tmp-file (insert body)) > + newbody) > + (with-temp-file tmp-file > + (insert (mapconcat 'identity > + (butlast lines) "\n"))) > + (concat newbody "\n" lastline))) > + ) nitpick: Move this trailing paren to the previous line. Also, it'd be nice to add a few tests to testing/lisp/test-ob-python.el. Thanks for working on this. -- Kyle ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: ob-python newline & indentation behavior 2017-11-20 6:25 ` Kyle Meyer @ 2017-11-20 21:31 ` Jack Kamm 2017-11-21 4:04 ` Kyle Meyer 0 siblings, 1 reply; 19+ messages in thread From: Jack Kamm @ 2017-11-20 21:31 UTC (permalink / raw) To: Kyle Meyer; +Cc: emacs-orgmode [-- Attachment #1: Type: text/plain, Size: 1294 bytes --] Hi Kyle, In response to this: I can't think of a good solution, though. Stepping back a bit, I think > it's unfortunate that python blocks handle ":results value" differently > depending on whether the block is hooked up to a session or not. For > non-sessions, you have to use return. Using the same approach > (org-babel-python-wrapper-method) for ":session :results value", we > could then get the return value reliably, but the problem with this > approach is that any variables defined in a ":results value" block > wouldn't be defined in the session after executing the block because the > code is wrapped in a function. > How about if we used the "globals()" and "locals()" functions in Python? Something like this at the end of the wrapper block, before return: for k, v in locals().items(): globals()[k] = v I think this would work a lot better than the current approach. Another bug with the current approach is that it breaks if common idioms like "for _ in range(10)" are used. ("_" is used to inspect the last output of the shell, an obscure feature I hadn't known about until now). Thanks for reviewing my suggested changes. Might be a few days until I can submit a new patch but I will incorporate your other suggestions. Waiting for the FSF forms anyways. Jack [-- Attachment #2: Type: text/html, Size: 2238 bytes --] ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: ob-python newline & indentation behavior 2017-11-20 21:31 ` Jack Kamm @ 2017-11-21 4:04 ` Kyle Meyer 2017-11-21 8:28 ` Jack Kamm 0 siblings, 1 reply; 19+ messages in thread From: Kyle Meyer @ 2017-11-21 4:04 UTC (permalink / raw) To: Jack Kamm; +Cc: emacs-orgmode Jack Kamm <jackkamm@gmail.com> writes: > In response to this: > >> I can't think of a good solution, though. Stepping back a bit, I think >> it's unfortunate that python blocks handle ":results value" differently >> depending on whether the block is hooked up to a session or not. For >> non-sessions, you have to use return. Using the same approach >> (org-babel-python-wrapper-method) for ":session :results value", we >> could then get the return value reliably, but the problem with this >> approach is that any variables defined in a ":results value" block >> wouldn't be defined in the session after executing the block because the >> code is wrapped in a function. > > How about if we used the "globals()" and "locals()" functions in Python? > > Something like this at the end of the wrapper block, before return: > > for k, v in locals().items(): > globals()[k] = v Hmm, placing that code "before return" is a problem. Like with non-session ":results value" blocks, the user would be responsible for inserting the return (or even multiple return's), so we can't know where to insert the above code without parsing the block :/ > Another bug with the current approach is that it breaks if common idioms > like "for _ in range(10)" are used. ("_" is used to inspect the last output > of the shell, an obscure feature I hadn't known about until now). Right. Also, IIRC the built-in interactive python and ipython treat multiline blocks differently. With if True: "ipython ignores my existence" the built-in shell binds "_" to the string's value, but ipython doesn't. -- Kyle ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: ob-python newline & indentation behavior 2017-11-21 4:04 ` Kyle Meyer @ 2017-11-21 8:28 ` Jack Kamm 2017-11-26 2:45 ` Ista Zahn 2017-11-26 8:15 ` Jack Kamm 0 siblings, 2 replies; 19+ messages in thread From: Jack Kamm @ 2017-11-21 8:28 UTC (permalink / raw) To: Kyle Meyer; +Cc: emacs-orgmode [-- Attachment #1: Type: text/plain, Size: 2110 bytes --] Yes, I'm starting to see now how difficult it is to properly support ":session :results value". I would vote to remove it from ob-python... I think the patch still improves ":session :results output" so I will simplify it and restrict to that case, leaving ":session :results value" unchanged for now. Sorry for sending this twice Kyle, forgot to reply all. On 21 Nov 2017 4:04 am, "Kyle Meyer" <kyle@kyleam.com> wrote: > Jack Kamm <jackkamm@gmail.com> writes: > > > In response to this: > > > >> I can't think of a good solution, though. Stepping back a bit, I think > >> it's unfortunate that python blocks handle ":results value" differently > >> depending on whether the block is hooked up to a session or not. For > >> non-sessions, you have to use return. Using the same approach > >> (org-babel-python-wrapper-method) for ":session :results value", we > >> could then get the return value reliably, but the problem with this > >> approach is that any variables defined in a ":results value" block > >> wouldn't be defined in the session after executing the block because the > >> code is wrapped in a function. > > > > How about if we used the "globals()" and "locals()" functions in Python? > > > > Something like this at the end of the wrapper block, before return: > > > > for k, v in locals().items(): > > globals()[k] = v > > Hmm, placing that code "before return" is a problem. Like with > non-session ":results value" blocks, the user would be responsible for > inserting the return (or even multiple return's), so we can't know where > to insert the above code without parsing the block :/ > > > Another bug with the current approach is that it breaks if common idioms > > like "for _ in range(10)" are used. ("_" is used to inspect the last > output > > of the shell, an obscure feature I hadn't known about until now). > > Right. Also, IIRC the built-in interactive python and ipython treat > multiline blocks differently. With > > if True: > "ipython ignores my existence" > > the built-in shell binds "_" to the string's value, but ipython doesn't. > > -- > Kyle > [-- Attachment #2: Type: text/html, Size: 3197 bytes --] ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: ob-python newline & indentation behavior 2017-11-21 8:28 ` Jack Kamm @ 2017-11-26 2:45 ` Ista Zahn 2017-11-26 8:25 ` Jack Kamm 2017-11-26 8:15 ` Jack Kamm 1 sibling, 1 reply; 19+ messages in thread From: Ista Zahn @ 2017-11-26 2:45 UTC (permalink / raw) To: Jack Kamm; +Cc: emacs-orgmode Mailinglist ob-ipython[1] provides a working alternative: #+BEGIN_SRC jupyter-python :session :results output foo = 0 for _ in range(10): foo += 1 foo += 1 print(foo) #+END_SRC #+RESULTS: : 20 I've long wished that more org people would show ob-ipython some love. Letting jupyter handle things on the backend seems like it should simplifly things considerably. [1] https://github.com/gregsexton/ob-ipython Best, Ista On Tue, Nov 21, 2017 at 3:28 AM, Jack Kamm <jackkamm@gmail.com> wrote: > Yes, I'm starting to see now how difficult it is to properly support > ":session :results value". I would vote to remove it from ob-python... > > I think the patch still improves ":session :results output" so I will > simplify it and restrict to that case, leaving ":session :results value" > unchanged for now. > > Sorry for sending this twice Kyle, forgot to reply all. > > On 21 Nov 2017 4:04 am, "Kyle Meyer" <kyle@kyleam.com> wrote: >> >> Jack Kamm <jackkamm@gmail.com> writes: >> >> > In response to this: >> > >> >> I can't think of a good solution, though. Stepping back a bit, I think >> >> it's unfortunate that python blocks handle ":results value" differently >> >> depending on whether the block is hooked up to a session or not. For >> >> non-sessions, you have to use return. Using the same approach >> >> (org-babel-python-wrapper-method) for ":session :results value", we >> >> could then get the return value reliably, but the problem with this >> >> approach is that any variables defined in a ":results value" block >> >> wouldn't be defined in the session after executing the block because >> >> the >> >> code is wrapped in a function. >> > >> > How about if we used the "globals()" and "locals()" functions in Python? >> > >> > Something like this at the end of the wrapper block, before return: >> > >> > for k, v in locals().items(): >> > globals()[k] = v >> >> Hmm, placing that code "before return" is a problem. Like with >> non-session ":results value" blocks, the user would be responsible for >> inserting the return (or even multiple return's), so we can't know where >> to insert the above code without parsing the block :/ >> >> > Another bug with the current approach is that it breaks if common idioms >> > like "for _ in range(10)" are used. ("_" is used to inspect the last >> > output >> > of the shell, an obscure feature I hadn't known about until now). >> >> Right. Also, IIRC the built-in interactive python and ipython treat >> multiline blocks differently. With >> >> if True: >> "ipython ignores my existence" >> >> the built-in shell binds "_" to the string's value, but ipython doesn't. >> >> -- >> Kyle ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: ob-python newline & indentation behavior 2017-11-26 2:45 ` Ista Zahn @ 2017-11-26 8:25 ` Jack Kamm 0 siblings, 0 replies; 19+ messages in thread From: Jack Kamm @ 2017-11-26 8:25 UTC (permalink / raw) To: Ista Zahn; +Cc: emacs-orgmode Mailinglist [-- Attachment #1: Type: text/plain, Size: 3556 bytes --] ob-ipython has a bug which renders it basically unusable to me -- it crashes when trying to interrupt the kernel with C-c C-c, which is something I often find myself needing to do (see https://github.com/gregsexton/ob-ipython/issues/115). However I agree that ob-ipython is very promising and could lead to org-babel becoming a viable jupyter notebook alternative in the future. Having :async execution is especially cool, if somewhat buggy right now. I hope ob-ipython continues to improve. Nevertheless, I would like ob-python to work properly, regardless of ob-ipython, as I like being able to use an python session in emacs without jupyter. On Sun, Nov 26, 2017 at 2:45 AM, Ista Zahn <istazahn@gmail.com> wrote: > ob-ipython[1] provides a working alternative: > > #+BEGIN_SRC jupyter-python :session :results output > foo = 0 > for _ in range(10): > foo += 1 > > foo += 1 > > print(foo) > #+END_SRC > > #+RESULTS: > : 20 > > I've long wished that more org people would show ob-ipython some love. > Letting jupyter handle things on the backend seems like it should > simplifly things considerably. > > [1] https://github.com/gregsexton/ob-ipython > > Best, > Ista > > On Tue, Nov 21, 2017 at 3:28 AM, Jack Kamm <jackkamm@gmail.com> wrote: > > Yes, I'm starting to see now how difficult it is to properly support > > ":session :results value". I would vote to remove it from ob-python... > > > > I think the patch still improves ":session :results output" so I will > > simplify it and restrict to that case, leaving ":session :results value" > > unchanged for now. > > > > Sorry for sending this twice Kyle, forgot to reply all. > > > > On 21 Nov 2017 4:04 am, "Kyle Meyer" <kyle@kyleam.com> wrote: > >> > >> Jack Kamm <jackkamm@gmail.com> writes: > >> > >> > In response to this: > >> > > >> >> I can't think of a good solution, though. Stepping back a bit, I > think > >> >> it's unfortunate that python blocks handle ":results value" > differently > >> >> depending on whether the block is hooked up to a session or not. For > >> >> non-sessions, you have to use return. Using the same approach > >> >> (org-babel-python-wrapper-method) for ":session :results value", we > >> >> could then get the return value reliably, but the problem with this > >> >> approach is that any variables defined in a ":results value" block > >> >> wouldn't be defined in the session after executing the block because > >> >> the > >> >> code is wrapped in a function. > >> > > >> > How about if we used the "globals()" and "locals()" functions in > Python? > >> > > >> > Something like this at the end of the wrapper block, before return: > >> > > >> > for k, v in locals().items(): > >> > globals()[k] = v > >> > >> Hmm, placing that code "before return" is a problem. Like with > >> non-session ":results value" blocks, the user would be responsible for > >> inserting the return (or even multiple return's), so we can't know where > >> to insert the above code without parsing the block :/ > >> > >> > Another bug with the current approach is that it breaks if common > idioms > >> > like "for _ in range(10)" are used. ("_" is used to inspect the last > >> > output > >> > of the shell, an obscure feature I hadn't known about until now). > >> > >> Right. Also, IIRC the built-in interactive python and ipython treat > >> multiline blocks differently. With > >> > >> if True: > >> "ipython ignores my existence" > >> > >> the built-in shell binds "_" to the string's value, but ipython doesn't. > >> > >> -- > >> Kyle > [-- Attachment #2: Type: text/html, Size: 5080 bytes --] ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: ob-python newline & indentation behavior 2017-11-21 8:28 ` Jack Kamm 2017-11-26 2:45 ` Ista Zahn @ 2017-11-26 8:15 ` Jack Kamm 2017-11-27 4:00 ` Kyle Meyer 1 sibling, 1 reply; 19+ messages in thread From: Jack Kamm @ 2017-11-26 8:15 UTC (permalink / raw) To: Kyle Meyer; +Cc: emacs-orgmode [-- Attachment #1.1: Type: text/plain, Size: 2849 bytes --] Hello, Attached please find my revised patch based on Kyle's feedback. I simplified the behavior to send the whole block to the tmpfile (including the lastline), but restricted this change to ":results output" only (":results value" retains its old, broken behavior). I also added a test and fixed minor style issues. I haven't heard back from the FSF yet, any idea how long it takes to hear from them? Given how broken ":session :results value" is, and the difficulty in implementing it correctly, I agree with Kyle's suggestion to remove it. Should we start another thread to discuss this? Jack On Tue, Nov 21, 2017 at 8:28 AM, Jack Kamm <jackkamm@gmail.com> wrote: > Yes, I'm starting to see now how difficult it is to properly support > ":session :results value". I would vote to remove it from ob-python... > > I think the patch still improves ":session :results output" so I will > simplify it and restrict to that case, leaving ":session :results value" > unchanged for now. > > Sorry for sending this twice Kyle, forgot to reply all. > > On 21 Nov 2017 4:04 am, "Kyle Meyer" <kyle@kyleam.com> wrote: > >> Jack Kamm <jackkamm@gmail.com> writes: >> >> > In response to this: >> > >> >> I can't think of a good solution, though. Stepping back a bit, I think >> >> it's unfortunate that python blocks handle ":results value" differently >> >> depending on whether the block is hooked up to a session or not. For >> >> non-sessions, you have to use return. Using the same approach >> >> (org-babel-python-wrapper-method) for ":session :results value", we >> >> could then get the return value reliably, but the problem with this >> >> approach is that any variables defined in a ":results value" block >> >> wouldn't be defined in the session after executing the block because >> the >> >> code is wrapped in a function. >> > >> > How about if we used the "globals()" and "locals()" functions in Python? >> > >> > Something like this at the end of the wrapper block, before return: >> > >> > for k, v in locals().items(): >> > globals()[k] = v >> >> Hmm, placing that code "before return" is a problem. Like with >> non-session ":results value" blocks, the user would be responsible for >> inserting the return (or even multiple return's), so we can't know where >> to insert the above code without parsing the block :/ >> >> > Another bug with the current approach is that it breaks if common idioms >> > like "for _ in range(10)" are used. ("_" is used to inspect the last >> output >> > of the shell, an obscure feature I hadn't known about until now). >> >> Right. Also, IIRC the built-in interactive python and ipython treat >> multiline blocks differently. With >> >> if True: >> "ipython ignores my existence" >> >> the built-in shell binds "_" to the string's value, but ipython doesn't. >> >> -- >> Kyle >> > [-- Attachment #1.2: Type: text/html, Size: 4426 bytes --] [-- Attachment #2: 0001-ob-python.el-fix-session-results-output-multiline-be.patch --] [-- Type: text/x-patch, Size: 3376 bytes --] From e46458004b83b034cb1388e707e866e84809b420 Mon Sep 17 00:00:00 2001 From: Jack Kamm <jackkamm@gmail.com> Date: Sun, 26 Nov 2017 08:00:29 +0000 Subject: [PATCH] ob-python.el: fix :session :results output multiline behavior * ob-python.el (orb-babel-python-evaluate-session org-babel-python--exec-tmpfile): When :session :results output, send multiline code blocks to tmpfile and execute in Python with exec() * test-ob-python.el (test-ob-python/session-multiline): test for :session with multiple lines and indentation --- lisp/ob-python.el | 38 ++++++++++++++++++++++++++++---------- testing/lisp/test-ob-python.el | 16 ++++++++++++++++ 2 files changed, 44 insertions(+), 10 deletions(-) diff --git a/lisp/ob-python.el b/lisp/ob-python.el index 60ec5fa47..3af5fb6bb 100644 --- a/lisp/ob-python.el +++ b/lisp/ob-python.el @@ -306,16 +306,19 @@ last statement in BODY, as elisp." (results (pcase result-type (`output - (mapconcat - #'org-trim - (butlast - (org-babel-comint-with-output - (session org-babel-python-eoe-indicator t body) - (funcall input-body body) - (funcall send-wait) (funcall send-wait) - (insert org-babel-python-eoe-indicator) - (funcall send-wait)) - 2) "\n")) + (let ((body (if (string-match-p ".\n+." body) ; Multiline + (org-babel-python--replace-body-tmpfile body) + body))) + (mapconcat + #'org-trim + (butlast + (org-babel-comint-with-output + (session org-babel-python-eoe-indicator t body) + (funcall input-body body) + (funcall send-wait) (funcall send-wait) + (insert org-babel-python-eoe-indicator) + (funcall send-wait)) + 2) "\n"))) (`value (let ((tmp-file (org-babel-temp-file "python-"))) (org-babel-comint-with-output @@ -340,6 +343,21 @@ last statement in BODY, as elisp." (substring string 1 -1) string)) +(defconst org-babel-python--exec-tmpfile + (concat + "__org_babel_python_fname = '%s'; " + "__org_babel_python_fh = open(__org_babel_python_fname); " + "exec(compile(" + "__org_babel_python_fh.read(), __org_babel_python_fname, 'exec'" + ")); " + "__org_babel_python_fh.close()")) + +(defun org-babel-python--replace-body-tmpfile (body) + "Place BODY in tmpfile, and return string to exec the tmpfile." + (let ((tmp-file (org-babel-temp-file "python-"))) + (with-temp-file tmp-file (insert body)) + (format org-babel-python--exec-tmpfile tmp-file))) + (provide 'ob-python) diff --git a/testing/lisp/test-ob-python.el b/testing/lisp/test-ob-python.el index d9fca220f..e77f9f36c 100644 --- a/testing/lisp/test-ob-python.el +++ b/testing/lisp/test-ob-python.el @@ -101,6 +101,22 @@ return x (should (equal '(("col") ("a") ("b")) (org-babel-execute-src-block))))) +(ert-deftest test-ob-python/session-multiline () + (run-python) + (sleep-for 0 10) + (org-test-with-temp-text " +#+begin_src python :session :results output + foo = 0 + for _ in range(10): + foo += 1 + + foo += 1 + + print(foo) +#+end_src" + (org-babel-next-src-block) + (should (equal "20" (org-babel-execute-src-block))))) + (provide 'test-ob-python) ;;; test-ob-python.el ends here -- 2.15.0 ^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: ob-python newline & indentation behavior 2017-11-26 8:15 ` Jack Kamm @ 2017-11-27 4:00 ` Kyle Meyer 2017-12-09 12:12 ` John Kamm 0 siblings, 1 reply; 19+ messages in thread From: Kyle Meyer @ 2017-11-27 4:00 UTC (permalink / raw) To: Jack Kamm; +Cc: emacs-orgmode Jack Kamm <jackkamm@gmail.com> writes: > I haven't heard back from the FSF yet, any idea how long it takes to hear > from them? Hmm, according to the site below, a response should come within 5 business days. That fits with my personal experience, but I'm not sure what's typical. Also, the delay might be Thanksgiving-related. <https://www.gnu.org/prep/maintain/html_node/Copyright-Papers.html> > Given how broken ":session :results value" is, and the difficulty in > implementing it correctly, I agree with Kyle's suggestion to remove it. > Should we start another thread to discuss this? I think that'd be good since 1) someone with an opinion on that topic might not notice it within this thread and 2) it's now orthogonal to this patch. > Subject: [PATCH] ob-python.el: fix :session :results output multiline behavior nit: To follow the convention used in the Org repo, "fix" would be capitalized. Also, just a preference, but the ".el" could be dropped. So, combining those two, that'd be ob-python: Fix :session :results output multiline behavior > * ob-python.el (orb-babel-python-evaluate-session s|ob-python.el|lisp/&| > org-babel-python--exec-tmpfile): When :session :results output, send > multiline code blocks to tmpfile and execute in Python with exec() nit: missing period Because org-babel-python--exec-tmpfile is a new function, I think it should be listed separately as (org-babel-python--exec-tmpfile): New function. > * test-ob-python.el (test-ob-python/session-multiline): test for > :session with multiple lines and indentation s|test-ob-python.el|testing/lisp/&| nits: s/test/Test/, missing period > +(defconst org-babel-python--exec-tmpfile > + (concat > + "__org_babel_python_fname = '%s'; " > + "__org_babel_python_fh = open(__org_babel_python_fname); " > + "exec(compile(" > + "__org_babel_python_fh.read(), __org_babel_python_fname, 'exec'" > + ")); " > + "__org_babel_python_fh.close()")) > + Looks good. Not a big deal either way, but I'd prefer org-babel-python--exec-tmpfile to be located directly below the other defconst templates (org-babel-python-{,pp-}wrapper-method). > +(defun org-babel-python--replace-body-tmpfile (body) > + "Place BODY in tmpfile, and return string to exec the tmpfile." > + (let ((tmp-file (org-babel-temp-file "python-"))) > + (with-temp-file tmp-file (insert body)) > + (format org-babel-python--exec-tmpfile tmp-file))) If you do the defconst move above, this function should go along with it. Purely a style preference, but for a short function like this that's only called in one place, I'd be in favor of dropping the defun and putting the code directly in org-babel-python-evaluate-session. > +(ert-deftest test-ob-python/session-multiline () Thanks. Passes on my end (and of course fails if I keep the test but revert the ob-python.el changes). > + (run-python) > + (sleep-for 0 10) I suspect you added this to avoid the issue of the starting prompt leaking into the output. I think that guess is right because, when I comment the two above lines out, it fails with an output string that contains the Python shell's startup text. I'm OK with this workaround, since the issue is unrelated to this patch, but should a FIXME comment be added above the run-python call to explain why it's there? Thanks. -- Kyle ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: ob-python newline & indentation behavior 2017-11-27 4:00 ` Kyle Meyer @ 2017-12-09 12:12 ` John Kamm 2017-12-18 8:22 ` Jack Kamm 0 siblings, 1 reply; 19+ messages in thread From: John Kamm @ 2017-12-09 12:12 UTC (permalink / raw) To: Kyle Meyer; +Cc: emacs-orgmode Kyle Meyer writes: > Hmm, according to the site below, a response should come within 5 > business days. That fits with my personal experience, but I'm not sure > what's typical. Also, the delay might be Thanksgiving-related. FSF sent me the papers which I signed and returned about a week ago, am just waiting for the final confirmation now. >> Given how broken ":session :results value" is, and the difficulty in >> implementing it correctly, I agree with Kyle's suggestion to remove it. >> Should we start another thread to discuss this? > > I think that'd be good since 1) someone with an opinion on that topic > might not notice it within this thread and 2) it's now orthogonal to > this patch. Great, I'll start another thread in a few days, summarizing the various options you laid out before. Another idea I had was to have some special "result" variable that could be assigned to. Anyways, we can discuss more in the new thread. > ... (suggestions to improve patch) I've incorporated all your suggestions/improvements into the patch, please see below. Thanks again for spending time to review it. From 40e961e349fdb347fbf9d59b3aca58163749f804 Mon Sep 17 00:00:00 2001 From: Jack Kamm <jackkamm@gmail.com> Date: Sat, 2 Dec 2017 09:03:00 +0000 Subject: [PATCH] ob-python: Fix :session :results output multiline behavior * lisp/ob-python.el (orb-babel-python-evaluate-session): When :session :results output, send multiline code blocks to tmpfile and execute in Python with exec(). (org-babel-python--exec-tmpfile): New function. * testing/lisp/test-ob-python.el (test-ob-python/session-multiline): Test for :session with multiple lines and indentation. --- lisp/ob-python.el | 36 ++++++++++++++++++++++++++---------- testing/lisp/test-ob-python.el | 17 +++++++++++++++++ 2 files changed, 43 insertions(+), 10 deletions(-) diff --git a/lisp/ob-python.el b/lisp/ob-python.el index 60ec5fa47..b3f50cfe5 100644 --- a/lisp/ob-python.el +++ b/lisp/ob-python.el @@ -239,6 +239,15 @@ def main(): open('%s', 'w').write( pprint.pformat(main()) )") +(defconst org-babel-python--exec-tmpfile + (concat + "__org_babel_python_fname = '%s'; " + "__org_babel_python_fh = open(__org_babel_python_fname); " + "exec(compile(" + "__org_babel_python_fh.read(), __org_babel_python_fname, 'exec'" + ")); " + "__org_babel_python_fh.close()")) + (defun org-babel-python-evaluate (session body &optional result-type result-params preamble) "Evaluate BODY as Python code." @@ -306,16 +315,23 @@ last statement in BODY, as elisp." (results (pcase result-type (`output - (mapconcat - #'org-trim - (butlast - (org-babel-comint-with-output - (session org-babel-python-eoe-indicator t body) - (funcall input-body body) - (funcall send-wait) (funcall send-wait) - (insert org-babel-python-eoe-indicator) - (funcall send-wait)) - 2) "\n")) + (let ((body (if (string-match-p ".\n+." body) ; Multiline + (let ((tmp-src-file (org-babel-temp-file + "python-"))) + (with-temp-file tmp-src-file (insert body)) + (format org-babel-python--exec-tmpfile + tmp-src-file)) + body))) + (mapconcat + #'org-trim + (butlast + (org-babel-comint-with-output + (session org-babel-python-eoe-indicator t body) + (funcall input-body body) + (funcall send-wait) (funcall send-wait) + (insert org-babel-python-eoe-indicator) + (funcall send-wait)) + 2) "\n"))) (`value (let ((tmp-file (org-babel-temp-file "python-"))) (org-babel-comint-with-output diff --git a/testing/lisp/test-ob-python.el b/testing/lisp/test-ob-python.el index d9fca220f..915b1bc77 100644 --- a/testing/lisp/test-ob-python.el +++ b/testing/lisp/test-ob-python.el @@ -101,6 +101,23 @@ return x (should (equal '(("col") ("a") ("b")) (org-babel-execute-src-block))))) +(ert-deftest test-ob-python/session-multiline () + ;; FIXME workaround to prevent starting prompt leaking into output + (run-python) + (sleep-for 0 10) + (org-test-with-temp-text " +#+begin_src python :session :results output + foo = 0 + for _ in range(10): + foo += 1 + + foo += 1 + + print(foo) +#+end_src" + (org-babel-next-src-block) + (should (equal "20" (org-babel-execute-src-block))))) + (provide 'test-ob-python) ;;; test-ob-python.el ends here -- 2.15.1 ^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: ob-python newline & indentation behavior 2017-12-09 12:12 ` John Kamm @ 2017-12-18 8:22 ` Jack Kamm 2017-12-18 11:44 ` Kyle Meyer 0 siblings, 1 reply; 19+ messages in thread From: Jack Kamm @ 2017-12-18 8:22 UTC (permalink / raw) To: Kyle Meyer; +Cc: emacs-orgmode [-- Attachment #1: Type: text/plain, Size: 141 bytes --] FSF sent me the signed papers :) I've attached them. Could Kyle or someone else merge in the patch, which I am reattaching? Thanks, Jack [-- Attachment #2: Kamm.1253922.EMACS.pdf --] [-- Type: application/pdf, Size: 1434020 bytes --] [-- Warning: decoded text below may be mangled, UTF-8 assumed --] [-- Attachment #3: 0001-ob-python-Fix-session-results-output-multiline-behav.patch --] [-- Type: text/x-patch, Size: 3441 bytes --] From bab63ad0941f45333208a8b690a38c0f51b568be Mon Sep 17 00:00:00 2001 From: Jack Kamm <jackkamm@gmail.com> Date: Sat, 2 Dec 2017 09:03:00 +0000 Subject: [PATCH] ob-python: Fix :session :results output multiline behavior * lisp/ob-python.el (orb-babel-python-evaluate-session): When :session :results output, send multiline code blocks to tmpfile and execute in Python with exec(). (org-babel-python--exec-tmpfile): New function. * testing/lisp/test-ob-python.el (test-ob-python/session-multiline): Test for :session with multiple lines and indentation. --- lisp/ob-python.el | 36 ++++++++++++++++++++++++++---------- testing/lisp/test-ob-python.el | 17 +++++++++++++++++ 2 files changed, 43 insertions(+), 10 deletions(-) diff --git a/lisp/ob-python.el b/lisp/ob-python.el index 60ec5fa47..b3f50cfe5 100644 --- a/lisp/ob-python.el +++ b/lisp/ob-python.el @@ -239,6 +239,15 @@ def main(): open('%s', 'w').write( pprint.pformat(main()) )") +(defconst org-babel-python--exec-tmpfile + (concat + "__org_babel_python_fname = '%s'; " + "__org_babel_python_fh = open(__org_babel_python_fname); " + "exec(compile(" + "__org_babel_python_fh.read(), __org_babel_python_fname, 'exec'" + ")); " + "__org_babel_python_fh.close()")) + (defun org-babel-python-evaluate (session body &optional result-type result-params preamble) "Evaluate BODY as Python code." @@ -306,16 +315,23 @@ last statement in BODY, as elisp." (results (pcase result-type (`output - (mapconcat - #'org-trim - (butlast - (org-babel-comint-with-output - (session org-babel-python-eoe-indicator t body) - (funcall input-body body) - (funcall send-wait) (funcall send-wait) - (insert org-babel-python-eoe-indicator) - (funcall send-wait)) - 2) "\n")) + (let ((body (if (string-match-p ".\n+." body) ; Multiline + (let ((tmp-src-file (org-babel-temp-file + "python-"))) + (with-temp-file tmp-src-file (insert body)) + (format org-babel-python--exec-tmpfile + tmp-src-file)) + body))) + (mapconcat + #'org-trim + (butlast + (org-babel-comint-with-output + (session org-babel-python-eoe-indicator t body) + (funcall input-body body) + (funcall send-wait) (funcall send-wait) + (insert org-babel-python-eoe-indicator) + (funcall send-wait)) + 2) "\n"))) (`value (let ((tmp-file (org-babel-temp-file "python-"))) (org-babel-comint-with-output diff --git a/testing/lisp/test-ob-python.el b/testing/lisp/test-ob-python.el index d9fca220f..915b1bc77 100644 --- a/testing/lisp/test-ob-python.el +++ b/testing/lisp/test-ob-python.el @@ -101,6 +101,23 @@ return x (should (equal '(("col") ("a") ("b")) (org-babel-execute-src-block))))) +(ert-deftest test-ob-python/session-multiline () + ;; FIXME workaround to prevent starting prompt leaking into output + (run-python) + (sleep-for 0 10) + (org-test-with-temp-text " +#+begin_src python :session :results output + foo = 0 + for _ in range(10): + foo += 1 + + foo += 1 + + print(foo) +#+end_src" + (org-babel-next-src-block) + (should (equal "20" (org-babel-execute-src-block))))) + (provide 'test-ob-python) ;;; test-ob-python.el ends here -- 2.15.1 ^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: ob-python newline & indentation behavior 2017-12-18 8:22 ` Jack Kamm @ 2017-12-18 11:44 ` Kyle Meyer 2017-12-19 5:11 ` Kyle Meyer 0 siblings, 1 reply; 19+ messages in thread From: Kyle Meyer @ 2017-12-18 11:44 UTC (permalink / raw) To: Jack Kamm; +Cc: emacs-orgmode Jack Kamm <jackkamm@gmail.com> writes: > FSF sent me the signed papers :) I've attached them. great :) > Could Kyle or someone else merge in the patch, which I am reattaching? I'll take a final look tonight (est). ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: ob-python newline & indentation behavior 2017-12-18 11:44 ` Kyle Meyer @ 2017-12-19 5:11 ` Kyle Meyer 0 siblings, 0 replies; 19+ messages in thread From: Kyle Meyer @ 2017-12-19 5:11 UTC (permalink / raw) To: Jack Kamm; +Cc: emacs-orgmode Kyle Meyer <kyle@kyleam.com> writes: > Jack Kamm <jackkamm@gmail.com> writes: > >> FSF sent me the signed papers :) I've attached them. > > great :) I've added your name here: http://orgmode.org/worg/org-contribute.html#contributors_with_fsf_papers >> Could Kyle or someone else merge in the patch, which I am reattaching? > > I'll take a final look tonight (est). Applied. Thanks for your work! -- Kyle ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: ob-python newline & indentation behavior 2017-11-18 22:15 ` Jack Kamm 2017-11-19 3:27 ` Martin Alsinet @ 2017-11-19 17:17 ` Kyle Meyer 1 sibling, 0 replies; 19+ messages in thread From: Kyle Meyer @ 2017-11-19 17:17 UTC (permalink / raw) To: Jack Kamm; +Cc: emacs-orgmode Jack Kamm <jackkamm@gmail.com> writes: > I adapted your old patch to the current master branch. I also extended it > to work for ":results value" (the original patch only worked for ":results > output"). I did this by not writing the last line of the code block to the > tmpfile, unless it is indented. > > I've never contributed before so would appreciate any comments on this > patch, and how to get it accepted. Cheers, Jack. Thanks for picking this up. I'll find time in the next couple of days to review your patch. For general information about contributing, take a look at http://orgmode.org/worg/org-contribute.html -- Kyle ^ permalink raw reply [flat|nested] 19+ messages in thread
end of thread, other threads:[~2017-12-19 5:11 UTC | newest] Thread overview: 19+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-11-18 12:02 ob-python newline & indentation behavior Jack Kamm 2017-11-18 15:05 ` Kyle Meyer 2017-11-18 22:15 ` Jack Kamm 2017-11-19 3:27 ` Martin Alsinet 2017-11-19 3:34 ` Martin Alsinet 2017-11-19 7:41 ` Jack Kamm 2017-11-20 6:25 ` Kyle Meyer 2017-11-20 21:31 ` Jack Kamm 2017-11-21 4:04 ` Kyle Meyer 2017-11-21 8:28 ` Jack Kamm 2017-11-26 2:45 ` Ista Zahn 2017-11-26 8:25 ` Jack Kamm 2017-11-26 8:15 ` Jack Kamm 2017-11-27 4:00 ` Kyle Meyer 2017-12-09 12:12 ` John Kamm 2017-12-18 8:22 ` Jack Kamm 2017-12-18 11:44 ` Kyle Meyer 2017-12-19 5:11 ` Kyle Meyer 2017-11-19 17:17 ` Kyle Meyer
Code repositories for project(s) associated with this public 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).