* Bug: ob-python mangles multiline :var values [9.3.6 (release_9.3.6-397-ga089600)] @ 2020-03-13 13:16 Štěpán Němec 2020-05-24 14:51 ` Jack Kamm 0 siblings, 1 reply; 8+ messages in thread From: Štěpán Němec @ 2020-03-13 13:16 UTC (permalink / raw) To: emacs-orgmode Recipe: ------- emacs -Q M-x load-library RET ob-python RET M-x org-mode RET #+begin_src python :var text="a\nb\nc" return text #+end_src #+RESULTS: : a : b : c Commentary: ----------- ob-python seems to prepend a TAB character to every line except for the first one. Emacs : GNU Emacs 28.0.50 (build 11, x86_64-pc-linux-gnu, GTK+ Version 3.24.14, cairo version 1.17.3) of 2020-03-11 Package: Org mode version 9.3.6 (release_9.3.6-397-ga08960 @ /home/stepnem/.emacs.d/lib/org/lisp/) ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: Bug: ob-python mangles multiline :var values [9.3.6 (release_9.3.6-397-ga089600)] 2020-03-13 13:16 Bug: ob-python mangles multiline :var values [9.3.6 (release_9.3.6-397-ga089600)] Štěpán Němec @ 2020-05-24 14:51 ` Jack Kamm 2020-05-27 18:26 ` Matthew Lundin 0 siblings, 1 reply; 8+ messages in thread From: Jack Kamm @ 2020-05-24 14:51 UTC (permalink / raw) To: Štěpán Němec, emacs-orgmode Hello, Thanks for reporting. I've just fixed this issue in master (commit 6149b6cb6). The problem was that ob-python adds tab indentation to the code body before putting it inside a main() function, which adds spurious indentation to multiline strings passed through :var. I fixed the issue by moving variable assignment from the code body to the code preamble, which is executed outside the main() function. Best, Jack Štěpán Němec <stepnem@gmail.com> writes: > Recipe: > ------- > > emacs -Q > M-x load-library RET ob-python RET > M-x org-mode RET > > #+begin_src python :var text="a\nb\nc" > return text > #+end_src > > #+RESULTS: > : a > : b > : c > > > Commentary: > ----------- > > ob-python seems to prepend a TAB character to every line except for the > first one. > > Emacs : GNU Emacs 28.0.50 (build 11, x86_64-pc-linux-gnu, GTK+ Version 3.24.14, cairo version 1.17.3) > of 2020-03-11 > Package: Org mode version 9.3.6 (release_9.3.6-397-ga08960 @ /home/stepnem/.emacs.d/lib/org/lisp/) ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: Bug: ob-python mangles multiline :var values [9.3.6 (release_9.3.6-397-ga089600)] 2020-05-24 14:51 ` Jack Kamm @ 2020-05-27 18:26 ` Matthew Lundin 2020-05-27 21:46 ` Jack Kamm 0 siblings, 1 reply; 8+ messages in thread From: Matthew Lundin @ 2020-05-27 18:26 UTC (permalink / raw) To: Jack Kamm, Štěpán Němec, emacs-orgmode Jack Kamm <jackkamm@gmail.com> writes: > Hello, > > Thanks for reporting. I've just fixed this issue in master (commit > 6149b6cb6). > > The problem was that ob-python adds tab indentation to the code body > before putting it inside a main() function, which adds spurious > indentation to multiline strings passed through :var. > > I fixed the issue by moving variable assignment from the code body to > the code preamble, which is executed outside the main() function. A heads up... I believe this changes the scope of the :var variables, since they previously were local to the main() function and now they are declared globally. After this change, some of my existing python source blocks (i.e., ones in which I attempt to assign a new value to a variable defined by :var) now generate the following error: Traceback (most recent call last): File "<stdin>", line 223, in <module> File "<stdin>", line 214, in main UnboundLocalError: local variable 'members' referenced before assignment I hesitate to call this a bug, since it would be fine to think of everything within the source block as local and the header :var declarations as global. But if this is the case, then I think it should be documented somewhere, especially since this change may break people's existing source blocks. And I suppose it would be worthwhile to ask: Is this change consistent with other org-babel modules? Is there a canonical way that org-babel handles scope? Best, Matt ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: Bug: ob-python mangles multiline :var values [9.3.6 (release_9.3.6-397-ga089600)] 2020-05-27 18:26 ` Matthew Lundin @ 2020-05-27 21:46 ` Jack Kamm 2020-05-28 14:39 ` Jack Kamm 0 siblings, 1 reply; 8+ messages in thread From: Jack Kamm @ 2020-05-27 21:46 UTC (permalink / raw) To: Matthew Lundin, Štěpán Němec, emacs-orgmode Hi Matt -- > A heads up... I believe this changes the scope of the :var variables, > since they previously were local to the main() function and now they are > declared globally. After this change, some of my existing python source > blocks (i.e., ones in which I attempt to assign a new value to a > variable defined by :var) now generate the following error: > > Traceback (most recent call last): > File "<stdin>", line 223, in <module> > File "<stdin>", line 214, in main > UnboundLocalError: local variable 'members' referenced before assignment Thanks for noticing this and pointing it out. This was an oversight on my end (I don't really use ":var" or non-session blocks). Unfortunately, the fix for the original bug will have to be a bit more complicated to avoid this error. I'll put it on my todo list, but if anyone wants to have a crack at it, that would be very welcome. We should also add a unit test for this regression so it doesn't happen again. > I hesitate to call this a bug, since it would be fine to think of > everything within the source block as local and the header :var > declarations as global. I think it's fair to call this a bug. I think it would be inconvenient to be unable to assign to these variables. Also, I did not intend to break any existing code with this. > And I suppose it would be worthwhile to ask: Is this change consistent > with other org-babel modules? Is there a canonical way that org-babel > handles scope? IMO everything ought to be in the same scope, and the user shouldn't have to think about the scope of the main() function that ob-python wraps the body in. Especially since non-session blocks are evaluated independently of other blocks, we really shouldn't have to think about their scope. I'm not sure if the Org manual provides any guidance here, but from a quick check I didn't see anything about it. Ideally I think everything should just be in global scope; however ob-python needs to use a wrapper function for the "return" keyword. ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: Bug: ob-python mangles multiline :var values [9.3.6 (release_9.3.6-397-ga089600)] 2020-05-27 21:46 ` Jack Kamm @ 2020-05-28 14:39 ` Jack Kamm 2020-06-06 18:24 ` Jack Kamm 0 siblings, 1 reply; 8+ messages in thread From: Jack Kamm @ 2020-05-28 14:39 UTC (permalink / raw) To: Matthew Lundin, Štěpán Němec, emacs-orgmode An update on this -- I decided to revert my fix for multiline Python variables. I left the unit test I added for this, but marked it as expected to fail. I'll try to submit a proper fix for this soon. However I've been swamped at work so I can't promise when (hopefully a couple weeks). In the meantime, I think it better to leave the original bug in place, rather than break any existing ob-python use cases. When I do get to this, I'll submit it as a patch to the list first, to make sure I don't accidentally cause a new bug. Best, Jack Jack Kamm <jackkamm@gmail.com> writes: > Hi Matt -- > >> A heads up... I believe this changes the scope of the :var variables, >> since they previously were local to the main() function and now they are >> declared globally. After this change, some of my existing python source >> blocks (i.e., ones in which I attempt to assign a new value to a >> variable defined by :var) now generate the following error: >> >> Traceback (most recent call last): >> File "<stdin>", line 223, in <module> >> File "<stdin>", line 214, in main >> UnboundLocalError: local variable 'members' referenced before assignment > > Thanks for noticing this and pointing it out. This was an oversight on > my end (I don't really use ":var" or non-session blocks). > > Unfortunately, the fix for the original bug will have to be a bit more > complicated to avoid this error. I'll put it on my todo list, but if > anyone wants to have a crack at it, that would be very welcome. > > We should also add a unit test for this regression so it doesn't happen > again. > >> I hesitate to call this a bug, since it would be fine to think of >> everything within the source block as local and the header :var >> declarations as global. > > I think it's fair to call this a bug. I think it would be inconvenient > to be unable to assign to these variables. Also, I did not intend to > break any existing code with this. > >> And I suppose it would be worthwhile to ask: Is this change consistent >> with other org-babel modules? Is there a canonical way that org-babel >> handles scope? > > IMO everything ought to be in the same scope, and the user shouldn't > have to think about the scope of the main() function that ob-python > wraps the body in. Especially since non-session blocks are evaluated > independently of other blocks, we really shouldn't have to think about > their scope. > > I'm not sure if the Org manual provides any guidance here, but from a > quick check I didn't see anything about it. > > Ideally I think everything should just be in global scope; however > ob-python needs to use a wrapper function for the "return" keyword. ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: Bug: ob-python mangles multiline :var values [9.3.6 (release_9.3.6-397-ga089600)] 2020-05-28 14:39 ` Jack Kamm @ 2020-06-06 18:24 ` Jack Kamm 2020-06-08 4:29 ` Kyle Meyer 0 siblings, 1 reply; 8+ messages in thread From: Jack Kamm @ 2020-06-06 18:24 UTC (permalink / raw) To: Matthew Lundin, Štěpán Němec, emacs-orgmode [-- Attachment #1: Type: text/plain, Size: 718 bytes --] Hi, Here's my second attempt to fix the issue. I'll wait a few days for comments before merging, to try and avoid causing a bug like last time. It turns out the problem was deeper than just the :var argument, multiline strings in the body were also getting mangled. For example, this block had the same problem as the original report: #+begin_src python text = """a b c """ return text #+end_src #+RESULTS: : a : b : c My fix this time was to use functions from python.el to indent, and to detect whether we are in a string and shouldn't be indented. I also added a couple more unit tests, one for multiline strings, and one for the variable scope/assignment issue that Matt reported. Cheers, Jack [-- Warning: decoded text below may be mangled, UTF-8 assumed --] [-- Attachment #2: 0001-ob-python.el-Fix-multiline-strings-in-non-session-re.patch --] [-- Type: text/x-patch, Size: 2904 bytes --] From 179178d39f6216172e1a070f570cf941f99b1a89 Mon Sep 17 00:00:00 2001 From: Jack Kamm <jackkamm@gmail.com> Date: Sat, 6 Jun 2020 10:59:23 -0700 Subject: [PATCH] ob-python.el: Fix multiline strings in non-session :results value * lisp/ob-python.el (org-babel-python-evaluate-external-process): Use functions from python.el to indent lines, avoiding multiline strings. * testing/lisp/test-ob-python.el (test-ob-python/multiline-var): Set test as expected to succeed. (test-ob-python/multiline-str): Add test for multiline string in body. (test-ob-python/header-var-assignment): Test that :var is in correct scope and can be assigned to. cf. https://orgmode.org/list/87tv009l9a.fsf@gmail.com/#t --- lisp/ob-python.el | 14 +++++++++----- testing/lisp/test-ob-python.el | 20 +++++++++++++++++++- 2 files changed, 28 insertions(+), 6 deletions(-) diff --git a/lisp/ob-python.el b/lisp/ob-python.el index dbcfac08d..622f69ce3 100644 --- a/lisp/ob-python.el +++ b/lisp/ob-python.el @@ -296,11 +296,15 @@ (defun org-babel-python-evaluate-external-process (if (member "pp" result-params) org-babel-python-pp-wrapper-method org-babel-python-wrapper-method) - (mapconcat - (lambda (line) (format "\t%s" line)) - (split-string (org-remove-indentation (org-trim body)) - "[\r\n]") - "\n") + (with-temp-buffer + (insert body) + (goto-char (point-min)) + (while (< (point) (point-max)) + (unless (python-syntax-context 'string) + (python-indent-shift-right (line-beginning-position) + (line-end-position))) + (forward-line 1)) + (buffer-string)) (org-babel-process-file-name tmp-file 'noquote)))) (org-babel-eval-read-file tmp-file)))))) (org-babel-result-cond result-params diff --git a/testing/lisp/test-ob-python.el b/testing/lisp/test-ob-python.el index 763942b16..e3f0571a1 100644 --- a/testing/lisp/test-ob-python.el +++ b/testing/lisp/test-ob-python.el @@ -174,7 +174,6 @@ (ert-deftest test-ob-python/assign-underscore () (org-babel-execute-src-block))))) (ert-deftest test-ob-python/multiline-var () - :expected-result :failed (should (equal "a\nb\nc" (org-test-with-temp-text "#+begin_src python :var text=\"a\\nb\\nc\" @@ -182,6 +181,25 @@ (ert-deftest test-ob-python/multiline-var () #+end_src" (org-babel-execute-src-block))))) +(ert-deftest test-ob-python/multiline-str () + (should + (equal "a\nb\nc" + (org-test-with-temp-text "#+begin_src python +text=\"a\\nb\\nc\" +return text +#+end_src" + (org-babel-execute-src-block))))) + +(ert-deftest test-ob-python/header-var-assignment () + (should + (equal "success" + (org-test-with-temp-text "#+begin_src python :var text=\"failure\" +text +text=\"success\" +return text +#+end_src" + (org-babel-execute-src-block))))) + (provide 'test-ob-python) ;;; test-ob-python.el ends here -- 2.27.0 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: Bug: ob-python mangles multiline :var values [9.3.6 (release_9.3.6-397-ga089600)] 2020-06-06 18:24 ` Jack Kamm @ 2020-06-08 4:29 ` Kyle Meyer 2020-06-10 4:05 ` Jack Kamm 0 siblings, 1 reply; 8+ messages in thread From: Kyle Meyer @ 2020-06-08 4:29 UTC (permalink / raw) To: Jack Kamm; +Cc: emacs-orgmode, Matthew Lundin, Štěpán Němec Jack Kamm writes: > My fix this time was to use functions from python.el to indent, and to > detect whether we are in a string and shouldn't be indented. > > I also added a couple more unit tests, one for multiline strings, and > one for the variable scope/assignment issue that Matt reported. Thanks for working on this. > diff --git a/lisp/ob-python.el b/lisp/ob-python.el > index dbcfac08d..622f69ce3 100644 > --- a/lisp/ob-python.el > +++ b/lisp/ob-python.el > @@ -296,11 +296,15 @@ (defun org-babel-python-evaluate-external-process > (if (member "pp" result-params) > org-babel-python-pp-wrapper-method > org-babel-python-wrapper-method) > - (mapconcat > - (lambda (line) (format "\t%s" line)) > - (split-string (org-remove-indentation (org-trim body)) > - "[\r\n]") > - "\n") > + (with-temp-buffer > + (insert body) > + (goto-char (point-min)) > + (while (< (point) (point-max)) nit-pick: In this code base, (not (eobp)) is the more common way to spell this. % git grep "(< (point) (point-max))" master | wc -l 1 % git grep "(not (eobp))" master | wc -l 44 > + (unless (python-syntax-context 'string) > + (python-indent-shift-right (line-beginning-position) > + (line-end-position))) These lead to byte-compiler warnings: Compiling /home/kyle/src/emacs/org-mode-devel/lisp/ob-python.el... In end of data: ob-python.el:391:1:Warning: the following functions are not known to be defined: python-syntax-context, python-indent-shift-right At the moment, python.el is only loaded for sessions (in org-babel-python-initiate-session-by-key), assuming org-babel-python-mode is set to `python'. With the above change, you're now using python.el functions in a non-session code path, so you'll need a (require 'python) somewhere. Also, just a note: I wondered whether python-syntax-context and python-indent-shift-right are available in Emacs 24.3, the minimum Emacs version we support. They are. ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: Bug: ob-python mangles multiline :var values [9.3.6 (release_9.3.6-397-ga089600)] 2020-06-08 4:29 ` Kyle Meyer @ 2020-06-10 4:05 ` Jack Kamm 0 siblings, 0 replies; 8+ messages in thread From: Jack Kamm @ 2020-06-10 4:05 UTC (permalink / raw) To: Kyle Meyer Cc: emacs-orgmode, Matthew Lundin, Štěpán Němec Hi Kyle, Thanks for the code review which was very helpful as always. I've fixed the style and compile errors that you noticed, and pushed the commit to master. Cheers, Jack ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2020-06-10 4:05 UTC | newest] Thread overview: 8+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2020-03-13 13:16 Bug: ob-python mangles multiline :var values [9.3.6 (release_9.3.6-397-ga089600)] Štěpán Němec 2020-05-24 14:51 ` Jack Kamm 2020-05-27 18:26 ` Matthew Lundin 2020-05-27 21:46 ` Jack Kamm 2020-05-28 14:39 ` Jack Kamm 2020-06-06 18:24 ` Jack Kamm 2020-06-08 4:29 ` Kyle Meyer 2020-06-10 4:05 ` Jack Kamm
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).