From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jack Kamm Subject: Re: ob-python newline & indentation behavior Date: Sun, 26 Nov 2017 08:15:29 +0000 Message-ID: References: <87h8trtyoa.fsf@kyleam.com> <8760a5tqjo.fsf@kyleam.com> <87r2ssp99s.fsf@kyleam.com> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="94eb2c0bb4e894c6af055ede638c" Return-path: Received: from eggs.gnu.org ([2001:4830:134:3::10]:54255) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1eIs68-0002PQ-PP for emacs-orgmode@gnu.org; Sun, 26 Nov 2017 03:15:34 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1eIs67-0008Ug-65 for emacs-orgmode@gnu.org; Sun, 26 Nov 2017 03:15:32 -0500 Received: from mail-yw0-x235.google.com ([2607:f8b0:4002:c05::235]:40592) by eggs.gnu.org with esmtps (TLS1.0:RSA_AES_128_CBC_SHA1:16) (Exim 4.71) (envelope-from ) id 1eIs66-0008Ua-Ty for emacs-orgmode@gnu.org; Sun, 26 Nov 2017 03:15:31 -0500 Received: by mail-yw0-x235.google.com with SMTP id t4so9222401ywf.7 for ; Sun, 26 Nov 2017 00:15:30 -0800 (PST) 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: Kyle Meyer Cc: emacs-orgmode@gnu.org --94eb2c0bb4e894c6af055ede638c Content-Type: multipart/alternative; boundary="94eb2c0bb4e894c6aa055ede638a" --94eb2c0bb4e894c6aa055ede638a Content-Type: text/plain; charset="UTF-8" Hello, Attached please find my revised patch based on Kyle's feedback. I simplified the behavior to send the whole block to the tmpfile (including the lastline), but restricted this change to ":results output" only (":results value" retains its old, broken behavior). I also added a test and fixed minor style issues. I haven't heard back from the FSF yet, any idea how long it takes to hear from them? 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? Jack On Tue, Nov 21, 2017 at 8:28 AM, Jack Kamm wrote: > Yes, I'm starting to see now how difficult it is to properly support > ":session :results value". I would vote to remove it from ob-python... > > I think the patch still improves ":session :results output" so I will > simplify it and restrict to that case, leaving ":session :results value" > unchanged for now. > > Sorry for sending this twice Kyle, forgot to reply all. > > On 21 Nov 2017 4:04 am, "Kyle Meyer" wrote: > >> Jack Kamm writes: >> >> > In response to this: >> > >> >> 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. >> > >> > How about if we used the "globals()" and "locals()" functions in Python? >> > >> > Something like this at the end of the wrapper block, before return: >> > >> > for k, v in locals().items(): >> > globals()[k] = v >> >> Hmm, placing that code "before return" is a problem. Like with >> non-session ":results value" blocks, the user would be responsible for >> inserting the return (or even multiple return's), so we can't know where >> to insert the above code without parsing the block :/ >> >> > Another bug with the current approach is that it breaks if common idioms >> > like "for _ in range(10)" are used. ("_" is used to inspect the last >> output >> > of the shell, an obscure feature I hadn't known about until now). >> >> Right. Also, IIRC the built-in interactive python and ipython treat >> multiline blocks differently. With >> >> if True: >> "ipython ignores my existence" >> >> the built-in shell binds "_" to the string's value, but ipython doesn't. >> >> -- >> Kyle >> > --94eb2c0bb4e894c6aa055ede638a Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable
Hello,

Attached please find = my revised patch based on Kyle's feedback. I simplified the behavior to= send the whole block to the tmpfile (including the lastline), but restrict= ed this change to ":results output" only (":results value&qu= ot; retains its old, broken behavior). I also added a test and fixed minor = style issues.

I haven't heard back from the FS= F yet, any idea how long it takes to hear from them?

Given how broken ":session :results value" is, and the difficu= lty in implementing it correctly, I agree with Kyle's suggestion to rem= ove it. Should we start another thread to discuss this?

<= /div>
Jack

On Tue, Nov 21, 2017 at 8:28 AM, Jack Kamm <<= a href=3D"mailto:jackkamm@gmail.com" target=3D"_blank">jackkamm@gmail.com> wrote:
Ye= s, I'm starting to see now how difficult it is to properly support &quo= t;:session :results value". I would vote to remove it from ob-python..= .

I think the patch still improves ":session :results output&quo= t; so I will simplify it and restrict to that case, leaving ":session = :results value" unchanged for now.

Sorry for sending this = twice Kyle, forgot to reply all.

=
On 21 Nov 2017 4:04 am, "K= yle Meyer" <ky= le@kyleam.com> wrote:
Jack Kamm <jackkamm@gmail.com> writes:

> In response to this:
>
>> I can't think of a good solution, though.=C2=A0 Stepping back = a bit, I think
>> it's unfortunate that python blocks handle ":results valu= e" differently
>> depending on whether the block is hooked up to a session or not.= =C2=A0 For
>> non-sessions, you have to use return.=C2=A0 Using the same approac= h
>> (org-babel-python-wrapper-method) for ":session :results= value", we
>> could then get the return value reliably, but the problem with thi= s
>> approach is that any variables defined in a ":results value&q= uot; block
>> wouldn't be defined in the session after executing the block b= ecause the
>> code is wrapped in a function.
>
> How about if we used the "globals()" and "locals()"= ; functions in Python?
>
> Something like this at the end of the wrapper block, before return: >
> for k, v in locals().items():
>=C2=A0 =C2=A0 =C2=A0globals()[k] =3D v

Hmm, placing that code "before return" is a problem.=C2=A0 Like w= ith
non-session ":results value" blocks, the user would be responsibl= e for
inserting the return (or even multiple return's), so we can't know = where
to insert the above code without parsing the block :/

> Another bug with the current approach is that it breaks if common idio= ms
> like "for _ in range(10)" are used. ("_" is used t= o inspect the last output
> of the shell, an obscure feature I hadn't known about until now).<= br>
Right.=C2=A0 Also, IIRC the built-in interactive python and ipython treat multiline blocks differently.=C2=A0 With

=C2=A0 =C2=A0 if True:
=C2=A0 =C2=A0 =C2=A0 =C2=A0 "ipython ignores my existence"

the built-in shell binds "_" to the string's value, but ipyth= on doesn't.

--
Kyle

--94eb2c0bb4e894c6aa055ede638a-- --94eb2c0bb4e894c6af055ede638c Content-Type: text/x-patch; charset="US-ASCII"; name="0001-ob-python.el-fix-session-results-output-multiline-be.patch" Content-Disposition: attachment; filename="0001-ob-python.el-fix-session-results-output-multiline-be.patch" Content-Transfer-Encoding: base64 X-Attachment-Id: f_jaghb3bh0 RnJvbSBlNDY0NTgwMDRiODNiMDM0Y2IxMzg4ZTcwN2U4NjZlODQ4MDliNDIwIE1vbiBTZXAgMTcg MDA6MDA6MDAgMjAwMQpGcm9tOiBKYWNrIEthbW0gPGphY2trYW1tQGdtYWlsLmNvbT4KRGF0ZTog U3VuLCAyNiBOb3YgMjAxNyAwODowMDoyOSArMDAwMApTdWJqZWN0OiBbUEFUQ0hdIG9iLXB5dGhv bi5lbDogZml4IDpzZXNzaW9uIDpyZXN1bHRzIG91dHB1dCBtdWx0aWxpbmUgYmVoYXZpb3IKCiog b2ItcHl0aG9uLmVsIChvcmItYmFiZWwtcHl0aG9uLWV2YWx1YXRlLXNlc3Npb24Kb3JnLWJhYmVs LXB5dGhvbi0tZXhlYy10bXBmaWxlKTogV2hlbiA6c2Vzc2lvbiA6cmVzdWx0cyBvdXRwdXQsIHNl bmQKbXVsdGlsaW5lIGNvZGUgYmxvY2tzIHRvIHRtcGZpbGUgYW5kIGV4ZWN1dGUgaW4gUHl0aG9u IHdpdGggZXhlYygpCiogdGVzdC1vYi1weXRob24uZWwgKHRlc3Qtb2ItcHl0aG9uL3Nlc3Npb24t bXVsdGlsaW5lKTogdGVzdCBmb3IKOnNlc3Npb24gd2l0aCBtdWx0aXBsZSBsaW5lcyBhbmQgaW5k ZW50YXRpb24KLS0tCiBsaXNwL29iLXB5dGhvbi5lbCAgICAgICAgICAgICAgfCAzOCArKysrKysr KysrKysrKysrKysrKysrKysrKysrLS0tLS0tLS0tLQogdGVzdGluZy9saXNwL3Rlc3Qtb2ItcHl0 aG9uLmVsIHwgMTYgKysrKysrKysrKysrKysrKwogMiBmaWxlcyBjaGFuZ2VkLCA0NCBpbnNlcnRp b25zKCspLCAxMCBkZWxldGlvbnMoLSkKCmRpZmYgLS1naXQgYS9saXNwL29iLXB5dGhvbi5lbCBi L2xpc3Avb2ItcHl0aG9uLmVsCmluZGV4IDYwZWM1ZmE0Ny4uM2FmNWZiNmJiIDEwMDY0NAotLS0g YS9saXNwL29iLXB5dGhvbi5lbAorKysgYi9saXNwL29iLXB5dGhvbi5lbApAQCAtMzA2LDE2ICsz MDYsMTkgQEAgbGFzdCBzdGF0ZW1lbnQgaW4gQk9EWSwgYXMgZWxpc3AuIgogICAgICAgICAgKHJl c3VsdHMKICAgICAgICAgICAocGNhc2UgcmVzdWx0LXR5cGUKICAgICAgICAgICAgIChgb3V0cHV0 Ci0gICAgICAgICAgICAgKG1hcGNvbmNhdAotICAgICAgICAgICAgICAjJ29yZy10cmltCi0gICAg ICAgICAgICAgIChidXRsYXN0Ci0gICAgICAgICAgICAgICAob3JnLWJhYmVsLWNvbWludC13aXRo LW91dHB1dAotICAgICAgICAgICAgICAgICAgIChzZXNzaW9uIG9yZy1iYWJlbC1weXRob24tZW9l LWluZGljYXRvciB0IGJvZHkpCi0gICAgICAgICAgICAgICAgIChmdW5jYWxsIGlucHV0LWJvZHkg Ym9keSkKLSAgICAgICAgICAgICAgICAgKGZ1bmNhbGwgc2VuZC13YWl0KSAoZnVuY2FsbCBzZW5k LXdhaXQpCi0gICAgICAgICAgICAgICAgIChpbnNlcnQgb3JnLWJhYmVsLXB5dGhvbi1lb2UtaW5k aWNhdG9yKQotICAgICAgICAgICAgICAgICAoZnVuY2FsbCBzZW5kLXdhaXQpKQotICAgICAgICAg ICAgICAgMikgIlxuIikpCisJICAgICAobGV0ICgoYm9keSAoaWYgKHN0cmluZy1tYXRjaC1wICIu XG4rLiIgYm9keSkgOyBNdWx0aWxpbmUKKwkJCSAgICAgKG9yZy1iYWJlbC1weXRob24tLXJlcGxh Y2UtYm9keS10bXBmaWxlIGJvZHkpCisJCQkgICBib2R5KSkpCisJICAgICAgIChtYXBjb25jYXQK KwkJIydvcmctdHJpbQorCQkoYnV0bGFzdAorCQkgKG9yZy1iYWJlbC1jb21pbnQtd2l0aC1vdXRw dXQKKwkJICAgICAoc2Vzc2lvbiBvcmctYmFiZWwtcHl0aG9uLWVvZS1pbmRpY2F0b3IgdCBib2R5 KQorCQkgICAoZnVuY2FsbCBpbnB1dC1ib2R5IGJvZHkpCisJCSAgIChmdW5jYWxsIHNlbmQtd2Fp dCkgKGZ1bmNhbGwgc2VuZC13YWl0KQorCQkgICAoaW5zZXJ0IG9yZy1iYWJlbC1weXRob24tZW9l LWluZGljYXRvcikKKwkJICAgKGZ1bmNhbGwgc2VuZC13YWl0KSkKKwkJIDIpICJcbiIpKSkKICAg ICAgICAgICAgIChgdmFsdWUKICAgICAgICAgICAgICAobGV0ICgodG1wLWZpbGUgKG9yZy1iYWJl bC10ZW1wLWZpbGUgInB5dGhvbi0iKSkpCiAgICAgICAgICAgICAgICAob3JnLWJhYmVsLWNvbWlu dC13aXRoLW91dHB1dApAQCAtMzQwLDYgKzM0MywyMSBAQCBsYXN0IHN0YXRlbWVudCBpbiBCT0RZ LCBhcyBlbGlzcC4iCiAgICAgICAoc3Vic3RyaW5nIHN0cmluZyAxIC0xKQogICAgIHN0cmluZykp CiAKKyhkZWZjb25zdCBvcmctYmFiZWwtcHl0aG9uLS1leGVjLXRtcGZpbGUKKyAgKGNvbmNhdAor ICAgIl9fb3JnX2JhYmVsX3B5dGhvbl9mbmFtZSA9ICclcyc7ICIKKyAgICJfX29yZ19iYWJlbF9w eXRob25fZmggPSBvcGVuKF9fb3JnX2JhYmVsX3B5dGhvbl9mbmFtZSk7ICIKKyAgICJleGVjKGNv bXBpbGUoIgorICAgIl9fb3JnX2JhYmVsX3B5dGhvbl9maC5yZWFkKCksIF9fb3JnX2JhYmVsX3B5 dGhvbl9mbmFtZSwgJ2V4ZWMnIgorICAgIikpOyAiCisgICAiX19vcmdfYmFiZWxfcHl0aG9uX2Zo LmNsb3NlKCkiKSkKKworKGRlZnVuIG9yZy1iYWJlbC1weXRob24tLXJlcGxhY2UtYm9keS10bXBm aWxlIChib2R5KQorICAiUGxhY2UgQk9EWSBpbiB0bXBmaWxlLCBhbmQgcmV0dXJuIHN0cmluZyB0 byBleGVjIHRoZSB0bXBmaWxlLiIKKyAgKGxldCAoKHRtcC1maWxlIChvcmctYmFiZWwtdGVtcC1m aWxlICJweXRob24tIikpKQorICAgICh3aXRoLXRlbXAtZmlsZSB0bXAtZmlsZSAoaW5zZXJ0IGJv ZHkpKQorICAgIChmb3JtYXQgb3JnLWJhYmVsLXB5dGhvbi0tZXhlYy10bXBmaWxlIHRtcC1maWxl KSkpCisKIChwcm92aWRlICdvYi1weXRob24pCiAKIApkaWZmIC0tZ2l0IGEvdGVzdGluZy9saXNw L3Rlc3Qtb2ItcHl0aG9uLmVsIGIvdGVzdGluZy9saXNwL3Rlc3Qtb2ItcHl0aG9uLmVsCmluZGV4 IGQ5ZmNhMjIwZi4uZTc3ZjlmMzZjIDEwMDY0NAotLS0gYS90ZXN0aW5nL2xpc3AvdGVzdC1vYi1w eXRob24uZWwKKysrIGIvdGVzdGluZy9saXNwL3Rlc3Qtb2ItcHl0aG9uLmVsCkBAIC0xMDEsNiAr MTAxLDIyIEBAIHJldHVybiB4CiAgICAgKHNob3VsZCAoZXF1YWwgJygoImNvbCIpICgiYSIpICgi YiIpKQogCQkgICAob3JnLWJhYmVsLWV4ZWN1dGUtc3JjLWJsb2NrKSkpKSkKIAorKGVydC1kZWZ0 ZXN0IHRlc3Qtb2ItcHl0aG9uL3Nlc3Npb24tbXVsdGlsaW5lICgpCisgIChydW4tcHl0aG9uKQor ICAoc2xlZXAtZm9yIDAgMTApCisgIChvcmctdGVzdC13aXRoLXRlbXAtdGV4dCAiCisjK2JlZ2lu X3NyYyBweXRob24gOnNlc3Npb24gOnJlc3VsdHMgb3V0cHV0CisgIGZvbyA9IDAKKyAgZm9yIF8g aW4gcmFuZ2UoMTApOgorICAgICAgZm9vICs9IDEKKworICAgICAgZm9vICs9IDEKKworICBwcmlu dChmb28pCisjK2VuZF9zcmMiCisgICAob3JnLWJhYmVsLW5leHQtc3JjLWJsb2NrKQorICAgKHNo b3VsZCAoZXF1YWwgIjIwIiAob3JnLWJhYmVsLWV4ZWN1dGUtc3JjLWJsb2NrKSkpKSkKKwogKHBy b3ZpZGUgJ3Rlc3Qtb2ItcHl0aG9uKQogCiA7OzsgdGVzdC1vYi1weXRob24uZWwgZW5kcyBoZXJl Ci0tIAoyLjE1LjAKCg== --94eb2c0bb4e894c6af055ede638c--