From mboxrd@z Thu Jan 1 00:00:00 1970 From: Kyle Meyer Subject: Re: [PATCH] Fix several issues with python session value blocks Date: Thu, 30 Jan 2020 05:50:00 +0000 Message-ID: <87k159fq7r.fsf@kyleam.com> References: <87blqubsdo.fsf@gmail.com> Mime-Version: 1.0 Content-Type: text/plain Return-path: Received: from eggs.gnu.org ([2001:470:142:3::10]:42921) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1ix2iO-0005gR-Vs for emacs-orgmode@gnu.org; Thu, 30 Jan 2020 00:50:10 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ix2iN-0007SX-2h for emacs-orgmode@gnu.org; Thu, 30 Jan 2020 00:50:08 -0500 Received: from pb-smtp1.pobox.com ([64.147.108.70]:65008) by eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32) (Exim 4.71) (envelope-from ) id 1ix2iM-0007Pk-N6 for emacs-orgmode@gnu.org; Thu, 30 Jan 2020 00:50:07 -0500 In-Reply-To: <87blqubsdo.fsf@gmail.com> 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-mx.org@gnu.org Sender: "Emacs-orgmode" To: Jack Kamm , emacs-orgmode@gnu.org Hi Jack, Thanks again for the patch. Testing it out a bit, your ast-based approach seems to work nicely. Jack Kamm 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, '', 'exec')) > + __org_babel_python_final = eval(compile(ast.Expression( > + __org_babel_python_final.value), '', 'eval')) > + else: > + exec(compile(__org_babel_python_ast, '', '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, '', 'exec')) @@ -261,9 +260,9 @@ (defconst org-babel-python--eval-tmpfile "import ast else: exec(compile(__org_babel_python_ast, '', '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)