From mboxrd@z Thu Jan 1 00:00:00 1970 From: Kyle Meyer Subject: Re: ob-python newline & indentation behavior Date: Mon, 20 Nov 2017 01:25:31 -0500 Message-ID: <8760a5tqjo.fsf@kyleam.com> References: <87h8trtyoa.fsf@kyleam.com> Mime-Version: 1.0 Content-Type: text/plain Return-path: Received: from eggs.gnu.org ([2001:4830:134:3::10]:40977) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1eGfWY-0007z2-6T for emacs-orgmode@gnu.org; Mon, 20 Nov 2017 01:25:43 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1eGfWU-0002Tb-W9 for emacs-orgmode@gnu.org; Mon, 20 Nov 2017 01:25:42 -0500 Received: from pb-smtp2.pobox.com ([64.147.108.71]:52615 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 1eGfWU-0002TR-PX for emacs-orgmode@gnu.org; Mon, 20 Nov 2017 01:25:38 -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: > Here is the new version of the patch: I haven't had any luck applying this patch to master. Perhaps your email client is altering the inline patch; you can instead attach the output file of 'git format-patch'. > From f009da37d3b7e2730abb8cbb10f4d07b3d456dd8 Mon Sep 17 00:00:00 2001 > > From: Jack Kamm > Date: Sun, 19 Nov 2017 07:13:56 +0000 > Subject: [PATCH] Squashed commit of the following: > > commit d1fe88a9f61a8e7082f08b7c190a29737bb655d5 > Author: Jack Kamm > Date: Sun Nov 19 07:08:31 2017 +0000 > > fix block ending in blank lines; send multiline blocks to tmpfile > > commit fcc5a7795e882716775c9d925b0cd5b657da041b > Author: Jack Kamm > Date: Sat Nov 18 22:40:31 2017 +0000 > > fix newlines and blanklines when sending codeblock to tmpfile > > commit a5d553ece9f6ee35cd1e273e554a21a19e80ec3c > Author: Jack Kamm > Date: Sat Nov 18 21:47:09 2017 +0000 > > fix newline/indentation issues in ob-python :session Please see for information on Org's convention for commit messages. In addition to following this format, ideally the message would contain a brief description of the problem the patch is fixing. > --- > lisp/ob-python.el | 29 +++++++++++++++++++++++++++++ > 1 file changed, 29 insertions(+) This patch probably passes the TINYCHANGE threshold, so please see for information about assigning copyright. > diff --git a/lisp/ob-python.el b/lisp/ob-python.el > index 60ec5fa47..c3dba1565 100644 > --- a/lisp/ob-python.el > +++ b/lisp/ob-python.el > @@ -303,6 +303,9 @@ last statement in BODY, as elisp." > (mapc (lambda (line) (insert line) (funcall > send-wait)) > (split-string body "[\r\n]")) > (funcall send-wait))) > + (body (if (string-match-p ".\n+." body) ;; Multiline nitpick: When a comments is on the same line as code, the convention is to use a single ";". I see your point that sending all multiline code through a temporary file is consistent with python.el's behavior. When you say that it "gives more consistent behavior for ":results output", I believe you're talking about problems with ">>>" and "..." markers leaking into the output. I agree that this change should be an improvement since I think most of the prompt issues are racy problems related to sending multiple lines. (I think there still might be a unrelated prompt issue regarding python.el's startup message leaking into the output of the first executed block.) Looking back at my summary of ob-python problems in , this seems like a good way to take the solution to problem #3 (indentation, multiline syntax errors) and apply it to problem #1 (markers leaking into the output). Nice, hadn't occurred to me. Assuming we do go this direction, I wonder if there's any ob-python code that can be cleaned up or simplified since the multiline processing logic is no longer needed. > + (org-babel-python--replace-body-tmpfile body) > + body)) > (results > (pcase result-type > (`output > @@ -340,6 +343,32 @@ last statement in BODY, as elisp." > (substring string 1 -1) > string)) > > + > +(defun org-babel-python--replace-body-tmpfile (body) > + "Place body in tmpfile, and return string to exec the tmpfile. nitpick: s/body/BODY/ here and below > +If last line of body is not indented, place it at end of exec string > +instead of tmpfile, so shell can see the result" nitpick: missing trailing period This is the part of the patch that I'm unsure about. I don't like that it * can fail if the last line is a non-indented, continued string * doesn't allow you to get a return value for commons things like conditionals, try-except blocks, context statements. I can't think of a good solution, though. Stepping back a bit, I think it's unfortunate that python blocks handle ":results value" differently depending on whether the block is hooked up to a session or not. For non-sessions, you have to use return. Using the same approach (org-babel-python-wrapper-method) for ":session :results value", we could then get the return value reliably, but the problem with this approach is that any variables defined in a ":results value" block wouldn't be defined in the session after executing the block because the code is wrapped in a function. So at this point I think the choice is between * restricting the return value to unindented last lines and not supporting continued strings in the last line * wrapping the return value with org-babel-python-wrapper-method and not supporting persistent session variables in that block * not supporting ":session :results value" > + (let* ((body (string-trim-right body)) > + (tmp-file (org-babel-temp-file "python-")) > + (lines (split-string body "[\r\n]")) > + (lastline (car (last lines))) > + (newbody (concat > + (format "__pyfilename = '%s'; " tmp-file) Since you're already using concat, I prefer to avoid the format here. Or you could add a defconst that just defines that format string and then only have a format call here. > + "__pyfile = open(__pyfilename); " Right, the leading underscores are good to reduce the risk of clobbering a variable that the user may have defined in the shell. Since this is generated code, I wonder if you should be even more specific and use ugly names like "__org_babel_python_fname" and "__org_babel_python_fh". > + "exec(compile(" > + "__pyfile.read(), __pyfilename, 'exec'" > + ")); " > + "__pyfile.close()"))) > + (if (string-match-p "^[ \t]" lastline) > + (progn > + (with-temp-file tmp-file (insert body)) > + newbody) > + (with-temp-file tmp-file > + (insert (mapconcat 'identity > + (butlast lines) "\n"))) > + (concat newbody "\n" lastline))) > + ) nitpick: Move this trailing paren to the previous line. Also, it'd be nice to add a few tests to testing/lisp/test-ob-python.el. Thanks for working on this. -- Kyle