From mboxrd@z Thu Jan 1 00:00:00 1970 From: Kyle Meyer Subject: Re: ob-python newline & indentation behavior Date: Sun, 26 Nov 2017 23:00:29 -0500 Message-ID: <87609wl6aq.fsf@kyleam.com> References: <87h8trtyoa.fsf@kyleam.com> <8760a5tqjo.fsf@kyleam.com> <87r2ssp99s.fsf@kyleam.com> Mime-Version: 1.0 Content-Type: text/plain Return-path: Received: from eggs.gnu.org ([2001:4830:134:3::10]:35104) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1eJAb0-0004Bw-SL for emacs-orgmode@gnu.org; Sun, 26 Nov 2017 23:00:40 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1eJAax-0004XS-QV for emacs-orgmode@gnu.org; Sun, 26 Nov 2017 23:00:38 -0500 Received: from pb-smtp2.pobox.com ([64.147.108.71]:53502 helo=sasl.smtp.pobox.com) by eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32) (Exim 4.71) (envelope-from ) id 1eJAax-0004XA-IV for emacs-orgmode@gnu.org; Sun, 26 Nov 2017 23:00:35 -0500 In-Reply-To: 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: Jack Kamm Cc: emacs-orgmode@gnu.org Jack Kamm 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. > 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