Jack Kamm <jackk...@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
