Re: [PATCH] Fix several issues with python session value blocks

[Kyle Meyer <kyle@kyleam.com>]

Hi Jack,

Thanks again for the patch.  Testing it out a bit, your ast-based
approach seems to work nicely.

Jack Kamm <jackkamm@gmail.com> writes:

> Subject: [PATCH] ob-python: Fix several issues with :session :results value
>
> * lisp/ob-python.el (org-babel-python-evaluate-session): Fix a few
> related issues with :session :results value blocks, including broken
> if-else statements, indented blocks with blank lines, and returning
> the wrong value when underscore has been used.  Uses the built-in ast

This is subjective, but I'd say up until "Uses [...]" would be good for
the changelog entry, and the remaining discussion could come as a
paragraph after.

The changelog should also include

    (org-babel-python--eval-tmpfile): New variable

or rather whatever the name ends up being, based on the discussion
below.

The content of your commit message is good as is, but I would be happy
to see a brief mention of what the approach is that's being replaced
before you describe the new approach with ast.

> python module to parse a source block, execute it, and evaluate the
> last line separately to return as a result.  Introduces a slight
> change in behavior, requiring that the last line must be a top-level
> statement if it's result is to be saved (otherwise, the result is
> None).

s/it's/its/

Wouldn't the more specific "top-level expression statement" be clearer
than "top-level statement"?

It's nice to see the change in behavior noted.  My uninformed guess is
that not many users were relying on getting the last value from
non-top-level expressions and that the issues you fix here very much
outweigh support for that, especially given that the change in behavior
is easy to accommodate.  However, this change in behavior probably
deserves a mention in ORG-NEWS.

Wrapped up in your statement about the top-level requirement, there is
also a change in behavior in that None will now show up under
"#+RESULTS:", as it already did with ":results value" for non-session
blocks.  I think the old behavior was likely just a limitation of
getting the old value from "_", and that the new behavior can be viewed
as a fix.

> diff --git a/lisp/ob-python.el b/lisp/ob-python.el
> index 823f6e63d..0b1073df5 100644
> --- a/lisp/ob-python.el
> +++ b/lisp/ob-python.el
> @@ -247,6 +247,24 @@ open('%s', 'w').write( pprint.pformat(main()) )")

Tangent: You can get better context for your diffs if you define a
custom xfuncname.  Something like

,----[ $XDG_CONFIG_HOME/git/attributes ]
| *.el     diff=lisp
`----

,----[ $XDG_CONFIG_HOME/git/config or ~/.gitconfig ]
| [diff "lisp"]
| 	xfuncname = "^(\\(.*)$"
`----

>     ")); "
>     "__org_babel_python_fh.close()"))
>  

> +(defconst org-babel-python--eval-tmpfile "import ast

nitpick: I think it'd read a bit nicer if you put `import ast' on a new
line with the same indentation level.  You could still avoid the string
having a new line at the start by using "\".

I don't find this variable name to be very clear, particularly when
taken along with the preceding org-babel-python--exec-tmpfile.  This new
variable is using both exec() and eval(), though granted the eval() is
the crucial bit for getting the value.  Perhaps a clearer name would be
something like org-babel-python--eval-ast.

> +with open('%s') as f:

Hmm, I'm nervous about breakage here if org-babel-temp-file returns
something with a single quote.  However, that's already a problem with
org-babel-python--exec-tmpfile used for ":results output", as well as
with a couple of other spots, so I think it'd be okay to punt on that
for now.

> +    __org_babel_python_ast = ast.parse(f.read())
> +
> +__org_babel_python_final = __org_babel_python_ast.body[-1]
> +try:
> +    if type(__org_babel_python_final) == ast.Expr:

I'd prefer `isinstance(__org_babel_python_final, ast.Expr)'.

> +        __org_babel_python_ast.body = __org_babel_python_ast.body[:-1]
> +        exec(compile(__org_babel_python_ast, '<string>', 'exec'))
> +        __org_babel_python_final = eval(compile(ast.Expression(
> +            __org_babel_python_final.value), '<string>', 'eval'))
> +    else:
> +        exec(compile(__org_babel_python_ast, '<string>', 'exec'))
> +        __org_babel_python_final = None

Okay, so if the last top-level node is an expression statement, convert
it to an Expression() that can be passed to eval() so that we can get
the return value.  Makes sense.

> +except Exception as e:
> +        __org_babel_python_final = e
> +        raise e")

nitpick: Your indentation here is one level too deep.

Hmm, we set the result to the exception on error, so the exception will
now show up under "#+RESULTS:".  As a not-really-babel user, my guess is
that that'd be a good thing, but I do wonder how other languages handle
exceptions.

Assuming we want to stick with that behavior, I suggest two changes:

  * Absorb the ast.parse() call into the try suite.  That way syntax
    errors in the source block are reported.

  * Set the value to some sort of traceback information rather than the
    exception itself.  If you set it to the exception, something like
    ValueError("i look like a proper result") will just show up under
    "#+RESULTS:" as "i look like a proper result" when it's fed to
    str().

I've included a patch at the end that sits on top of yours and does
those two things.  If it looks reasonable to you, please squash it into
your patch.

>  	 (input-body (lambda (body)
> -		       (dolist (line (split-string body "[\r\n]"))
> -			 ;; Insert a blank line to end an indent
> -			 ;; block.
> -			 (let ((curr-indent (string-match "\\S-" line)))
> -			   (if curr-indent
> -			       (progn
> -				 (when (< curr-indent last-indent)
> -				   (insert "")
> -				   (funcall send-wait))
> -				 (setq last-indent curr-indent))
> -			     (setq last-indent 0)))
> -			 (insert line)
> -			 (funcall send-wait))
> +		       (mapc (lambda (line) (insert line) (funcall send-wait))
> +			     (split-string body "[\r\n]"))

Let's stick with dolist rather than switching to mapc.  In addition to
leading to a more minimal/pleasant diff, it's in line with the
preference shown by changes like ef0178980 (org.el: Use `dolist' instead
of `mapc' + `lambda', 2016-01-10).

>              (`value
> -             (let ((tmp-file (org-babel-temp-file "python-")))
> +             (let ((tmp-results-file (org-babel-temp-file "python-"))
> +		   (body (let ((tmp-src-file (org-babel-temp-file
> +					      "python-")))
> +			   (with-temp-file tmp-src-file (insert body))
> +			   (format org-babel-python--eval-tmpfile
> +				   tmp-src-file))))
>                 (org-babel-comint-with-output
>                     (session org-babel-python-eoe-indicator nil body)
>                   (let ((comint-process-echoes nil))
>                     (funcall input-body body)
> -                   (funcall dump-last-value tmp-file
> -                            (member "pp" result-params))
> +		   (mapc

Same comment here about mapc vs dolist.

> +		    (lambda (statement) (insert statement) (funcall send-wait))
> +		    (if (member "pp" result-params)
> +			(list
> +			 "import pprint"
> +			 (format
> +			  "open('%s', 'w').write(pprint.pformat(__org_babel_python_final))"
> +			  (org-babel-process-file-name tmp-results-file 'noquote)))
> +		      (list (format "open('%s', 'w').write(str(__org_babel_python_final))"

Hmm, this str(), as well as the write(), could cause encoding errors on
Python 2 if __org_babel_python_final is a unicode() object.  I don't
think adding a compatibility kludge is worth the trouble given that
other spots in ob-python.el have similar issues and that Python 2 has
reached EOL.

-- >8 --
diff --git a/lisp/ob-python.el b/lisp/ob-python.el
index 0b1073df5..8cbcd54a3 100644
--- a/lisp/ob-python.el
+++ b/lisp/ob-python.el
@@ -248,11 +248,10 @@ (defconst org-babel-python--exec-tmpfile
    "__org_babel_python_fh.close()"))
 
 (defconst org-babel-python--eval-tmpfile "import ast
-with open('%s') as f:
-    __org_babel_python_ast = ast.parse(f.read())
-
-__org_babel_python_final = __org_babel_python_ast.body[-1]
 try:
+    with open('%s') as f:
+        __org_babel_python_ast = ast.parse(f.read())
+    __org_babel_python_final = __org_babel_python_ast.body[-1]
     if type(__org_babel_python_final) == ast.Expr:
         __org_babel_python_ast.body = __org_babel_python_ast.body[:-1]
         exec(compile(__org_babel_python_ast, '<string>', 'exec'))
@@ -261,9 +260,9 @@ (defconst org-babel-python--eval-tmpfile "import ast
     else:
         exec(compile(__org_babel_python_ast, '<string>', 'exec'))
         __org_babel_python_final = None
-except Exception as e:
-        __org_babel_python_final = e
-        raise e")
+except Exception:
+    from traceback import format_exc
+    __org_babel_python_final = format_exc()")
 
 (defun org-babel-python-evaluate
   (session body &optional result-type result-params preamble)