From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mp12.migadu.com ([2001:41d0:8:6d80::]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits)) by ms0.migadu.com with LMTPS id YEPuGPMq0WFs1QAAgWs5BA (envelope-from ) for ; Sun, 02 Jan 2022 05:32:51 +0100 Received: from aspmx1.migadu.com ([2001:41d0:8:6d80::]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits)) by mp12.migadu.com with LMTPS id 6Al7FfMq0WEzNwAAauVa8A (envelope-from ) for ; Sun, 02 Jan 2022 05:32:51 +0100 Received: from lists.gnu.org (lists.gnu.org [209.51.188.17]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by aspmx1.migadu.com (Postfix) with ESMTPS id 6B45A299FC for ; Sun, 2 Jan 2022 05:32:50 +0100 (CET) Received: from localhost ([::1]:54852 helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1n3sY5-000156-Fn for larch@yhetil.org; Sat, 01 Jan 2022 23:32:49 -0500 Received: from eggs.gnu.org ([209.51.188.92]:43728) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1n3sXT-00014Z-Uk for emacs-orgmode@gnu.org; Sat, 01 Jan 2022 23:32:12 -0500 Received: from sender4-op-o18.zoho.com ([136.143.188.18]:17826) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1n3sXR-0002ae-Dd for emacs-orgmode@gnu.org; Sat, 01 Jan 2022 23:32:11 -0500 ARC-Seal: i=1; a=rsa-sha256; t=1641097923; cv=none; d=zohomail.com; s=zohoarc; b=MWj0sIkQL0gl+Brv3khypvtcN6zIfcgUv3+x9CnMbuVxDdX3hmenmgE8X3OdPmxXSyx+6VcXZPrpnEsVSQmeffYyOOeApWDUDnfpTE2seZSX/kUlWyaQcOyz5S7GNhitcwQI/42DIYUe1FRYwazt/ymiex4UbKBSA8o00ACTmCs= ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=zohomail.com; s=zohoarc; t=1641097923; h=Content-Type:Content-Transfer-Encoding:Cc:Date:From:In-Reply-To:MIME-Version:Message-ID:References:Subject:To; bh=4ylefw3jrd91eHuYVGoqSqfDBWZvHsBuEJjgwUpEfgQ=; b=MwTxCowEi9kRwfJv4aWHkri0GLzuKmw7N0AG+ZAgjNSSnAxDThp1f9san6dgbhfKSlafEA1IabfptGeiq8dzGDg8kZF2mbN5BQIi3c2yl6Q2Sh76t0as8kgj7g8nVTIg+4FFYw4twPRppMgJRnzp8X2SBHEIoZ4j2U0afa3Wxcs= ARC-Authentication-Results: i=1; mx.zohomail.com; dkim=pass header.i=excalamus.com; spf=pass smtp.mailfrom=matt@excalamus.com; dmarc=pass header.from= DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; t=1641097923; s=zmail; d=excalamus.com; i=matt@excalamus.com; h=Date:From:To:Cc:Message-ID:In-Reply-To:References:Subject:MIME-Version:Content-Type:Content-Transfer-Encoding; bh=4ylefw3jrd91eHuYVGoqSqfDBWZvHsBuEJjgwUpEfgQ=; b=YWR5cHzY5EEtZ5GU4sVjzLTXjKGhTFR5noj/Voi7CowMmWICLMh/wNBmIDQfYFTm +Y6XDKcItMrFdGxkuSIz8Wf2owouP40tDTPJHeTKymKOPiwTau1EMwi72InljFQMFNH k0C0yzvwVsjjixPHphmRapwAqgEvklaLpVsOogAY= Received: from mail.zoho.com by mx.zohomail.com with SMTP id 1641097920822828.7070152511011; Sat, 1 Jan 2022 20:32:00 -0800 (PST) Date: Sat, 01 Jan 2022 23:32:00 -0500 From: Matt To: "Thomas S. Dye" Message-ID: <17e190f0110.dffbf4bc1032.6816869643164380190@excalamus.com> In-Reply-To: <8735m8ifvr.fsf@tsdye.online> References: <17d48f09729.ddbece82213631.8340272801066201269@excalamus.com> <87pmqs58mx.fsf@gmail.com> <17d53474fd6.c0a62421660695.6561002670353136607@excalamus.com> <87lf13pcme.fsf@gmail.com> <17d8ddd76f7.d167f605179643.3635445903460764440@excalamus.com> <87bl1u49ru.fsf@tsdye.online> <17dcc5a0e3e.d8ca0ef6361712.4976360594738663013@excalamus.com> <875yr4iu3a.fsf@tsdye.online> <17e11edb77e.adb20ab5289948.958495178633735939@excalamus.com> <8735m8ifvr.fsf@tsdye.online> Subject: Re: [PATCH] ob-shell-test, test-ob-shell and introduction MIME-Version: 1.0 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable Importance: Medium User-Agent: Zoho Mail X-Mailer: Zoho Mail Received-SPF: pass client-ip=136.143.188.18; envelope-from=matt@excalamus.com; helo=sender4-op-o18.zoho.com X-Spam_score_int: -1 X-Spam_score: -0.2 X-Spam_bar: / X-Spam_report: (-0.2 / 5.0 requ) DKIM_SIGNED=0.1, DKIM_VALID=-0.1, DKIM_VALID_AU=-0.1, DKIM_VALID_EF=-0.1, RCVD_IN_DNSWL_NONE=-0.0001, SPF_HELO_NONE=0.001, SPF_PASS=-0.001 autolearn=unavailable autolearn_force=no X-Spam_action: no action X-BeenThere: emacs-orgmode@gnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: "General discussions about Org-mode." List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: emacs-orgmode , Timothy Errors-To: emacs-orgmode-bounces+larch=yhetil.org@gnu.org Sender: "Emacs-orgmode" X-Migadu-Flow: FLOW_IN X-Migadu-Country: US ARC-Message-Signature: i=2; a=rsa-sha256; c=relaxed/relaxed; d=yhetil.org; s=key1; t=1641097971; h=from:from:sender:sender:reply-to:subject:subject:date:date: message-id:message-id:to:to:cc:cc:mime-version:mime-version: content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references:list-id:list-help: list-unsubscribe:list-subscribe:list-post:dkim-signature; bh=4ylefw3jrd91eHuYVGoqSqfDBWZvHsBuEJjgwUpEfgQ=; b=IrJbJxF39F5kWeYg7M3akE1oXTOPWpn/ka6YA1BxlioWRA0xufhvPl1lQQ+PfckdBVmlDR +NTZIdzEXDxRtCH3bS6FX/u2CQZaEVuNVOtyEKHi/z26wsPFm1ofLIIdeSPkjzGLqWdiSd awNljCMGQFVbp6UKtqaKVrnbSNrcn5JhOCq7ycREBPDwuM57Roxf7ia0AfqcHBxl/JVjol //6txV071CJ4mJ3/o2oCAZYRCGn6MhGR5Cc/kPyzh44yeKZul9v2kYfMjBPjHZ8XahcR0+ u56jIxKblgRTWsVBUauZTbtc1aE4Pix9BBOwO93IskWgELKijejvoJ6OWwNTig== ARC-Seal: i=2; s=key1; d=yhetil.org; t=1641097971; a=rsa-sha256; cv=fail; b=qSCpSneGvVrsnO3sDu8DYtR+P4U84GpY37iWC0gpbilKVwhp4/danaXoPu/4LB2r0RZv4/ +qGhLB5/StMnsPT/ZoCSvoZl8zz3L9FdjZKHpvbQWV7ViFGgSb6xbm8g5WZTF5jyxHSUN1 wHkE78+O/OVUDpAjk/Xdzq8xF+UvH26h2uHFpkG/5NikmJpL9m46X5PaDoNnBZs3+hebWZ 4ctRm63etf772N6Rpbs5PDNwo3BIbedX80MIYDCuqumy5OinUV88EiDNhUWujTIccgYpaL NtmmDTUF+ihzLmwmX1CaTbl0roSKQaG061gWrNjj+rd54I5CIKsBBmn9JdU3mA== ARC-Authentication-Results: i=2; aspmx1.migadu.com; dkim=fail ("headers rsa verify failed") header.d=excalamus.com header.s=zmail header.b=YWR5cHzY; arc=reject ("signature check failed: fail, {[1] = sig:zohomail.com:reject}"); dmarc=none; spf=pass (aspmx1.migadu.com: domain of "emacs-orgmode-bounces+larch=yhetil.org@gnu.org" designates 209.51.188.17 as permitted sender) smtp.mailfrom="emacs-orgmode-bounces+larch=yhetil.org@gnu.org" X-Migadu-Spam-Score: -1.07 Authentication-Results: aspmx1.migadu.com; dkim=fail ("headers rsa verify failed") header.d=excalamus.com header.s=zmail header.b=YWR5cHzY; arc=reject ("signature check failed: fail, {[1] = sig:zohomail.com:reject}"); dmarc=none; spf=pass (aspmx1.migadu.com: domain of "emacs-orgmode-bounces+larch=yhetil.org@gnu.org" designates 209.51.188.17 as permitted sender) smtp.mailfrom="emacs-orgmode-bounces+larch=yhetil.org@gnu.org" X-Migadu-Queue-Id: 6B45A299FC X-Spam-Score: -1.07 X-Migadu-Scanner: scn1.migadu.com X-TUID: 7Ly4UmEfefyM Apologies for the book. I've been sitting on this stuff for two months and= am wondering how to proceed. IANAL but AFAIK/CT, my contract contains an exception for making contributi= ons to projects like Org. I've gotten confirmation from my manager and by H= R. However, until the CEO signs the FSF disclaimer, I can't officially con= tribute. I'm confident that I can publish changes (e.g. to a personal webs= ite); the FSF just can't accept my changes (yet). I could start working on ob-shell.el separately now and publish that myself= . It's not clear how I would then contribute those changes back once I'm cl= eared by the FSF. I'm inclined towards some refactoring and I'm not sure h= ow I could break that down in such a way that if it took two more months to= get the copyright stuff worked out that anyone could make sense of the cha= nges. I would much prefer to gradually submit patches, discuss them, and t= hen eventually have them merged in turn. If I have a heap of changes elsew= here, I feel like it would be harder to have a conversion about them. Regardless, I think I should define test cases first. If those are conside= red valid, then any refactoring would be moot if they pass, right? With (a= greed upon) test cases, it shouldn't matter if I refactor as long as functi= onality remains the same? Overall, what advice do you have? It looks to me like ob-shell.el could use some love in other respects besid= es async evaluation. I've detailed where below, partly for my own organiza= tion and partly for posterity, but mainly because this isn't my house, so t= o speak, and I don't want to barge in and start rearranging the furniture a= nd eating whatever's in the fridge. Or, is it like Worg in that once I hav= e the keys I can drive where I like, so long as there're no crashes? I'm interested in people's thoughts on these notes on ob-shell.el: * Tests There are several code paths, like shebang, cmdline, and basic execution, w= hich don't always have something to do with one another in the code. Havin= g tests would be really helpful to make sure everything jives. Before doin= g anything with the code base, I intend to create tests for all the functio= nality. * 2D-array I documented two options for the =3D:var=3D header[fn:1]. The ob-shell.el c= ode actually defines a third option for 2D-arrays. I couldn't get it to wo= rk. It was one of several things not documented anywhere, at least as far = as I could find, and which I had to figure out straight from the code. Bet= ween not being great at shell scripting and having a hard time deciphering = that ob-shell.el code, I'm not sure 2D-arrays are actually fully or correct= ly implemented. * M-up behavior <> The =3Dorg-babel-load-session:shell=3D code path only works when M-up is us= ed on a code block[fn:2]. Is M-up actually documented anywhere? Furthermor= e, the =3Dorg-babel-load-session:shell=3D only works for the "shell" langua= ge, which is not actually a "proper" shell (i.e. it's not listed in =3Dorg-= babel-shell-names=3D). The M-up defaults to using $ESHELL or shell-file-nam= e through the =3Dshell=3D command. For example, try calling M-up on these: #+comment: (opaquely) calls the system shell #+begin_src shell :session my-shell echo "hello, world!" #+end_src #+comment: fails #+begin_src sh :session my-sh echo "hello, world!" #+end_src #+comment: fails #+begin_src bash :session my-bash echo "hello, world!" #+end_src To fix this, there needs to be an =3Dorg-babel-load-session:=3D for e= ach language in =3Dorg-babel-shell-names=3D. This would probably make the = most sense in =3Dorg-babel-shell-initialize=3D. However, that function [[o= rg-babel-shell-initialize][needs attention]]. * Refactoring <> The ob-shell.el code appears inconsistent to me. Clearly, this is somewhat= subjective. I've tried to give a rationale for each point to make it less= so. My goal is to be maintainer of ob-shell.el, but that's not forever. = These things were an obstacle for me and my aim is to remove them for the n= ext person. ** =3Dorg-babel-shell-initialize=3D <> As alluded to elsewhere, =3Dorg-babel-shell-initialize=3D doesn't appear to= adhere to the (elisp) Coding Conventions, #+begin_quote =E2=80=A2 Constructs that define a function or variable should be macros= , not functions, and their names should start with =E2=80=98define-=E2=80= =99. The macro should receive the name to be defined as the first argument= . That will help various tools find the definition automatically. Avoid c= onstructing the names in the macro itself, since that would confuse these t= ools. #+end_quote The =3Dorg-babel-shell-initialize=3D function defines =3Dorg-babel-execute:= =3D, =3Dorg-babel-variable-assignments:=3D, and =3Dorg-babel-de= fault-header-args:=3D for the "languages" in =3Dorg-babel-shell-names= =3D. As it stands, that =3Dorg-babel-shell-initialize=3D is a function doe= s no harm (aside from being confusing by way of straying from convention). = However, if the [[M-up][M-up]] issue is to be resolved, it seems to me tha= t =3Dorg-babel-shell-initialize=3D should be updated to match the conventio= n (i.e. be a macro). ** Organization Definitions are introduced in different orders. For example, the =3Dorg-ba= bel-shell-initialize=3D function whose sole purpose is to work with =3Dorg-= babel-shell-names=3D is defined before the reader knows what =3Dorg-babel-s= hell-names=3D is. Later, this pattern of defining the component pieces aft= er they're used is reversed. For example, =3Dorg-babel-load-session:shell= =3D relies on =3Dorg-babel-prep-session:shell=3D which is defined first. I= find defining terms before they're used makes a document more easy to comp= rehend than otherwise. I want to reorder the definitions. Similarly, some functions could use breaking out. The most important is pr= obably =3Dorg-babel-sh-evaluate=3D which handles the various header argumen= ts. There are various paths of execution each requiring separate logic, ye= t all live in the one large function. Breaking these out would allow them = to have separate docstrings and, I expect, be easier to understand since th= e logic of one would be (lexically) separated from the rest. Other functionality might be better served by consolidating functions. Ther= e's a lot of fiddly code dedicated to variable assignment. Actually, much o= f the ob-shell.el file is related to variable assignment. The assignments a= re done using separate functions, yet all apply to the same task. They'll n= ever be used for anything else. Do they need to be split out? Is there a = technical reason? I don't see one. Does it help comprehension? When split= out as they are, I found it hard to make sense of the context they're used= in since they're defined apart from the logic that uses them (i.e. what he= ader uses them, what form does the header take, etc.). I think it's worth = seeing if there's a better way to present that part of the code base. ** Naming The following apply to all shells, not just "sh" and should be updated to b= e "shell". This looks like cruft from when ob-shell.el was called ob-sh.el = AFAICT. - =3Dorg-babel-sh-evaluate=3D - =3Dorg-babel-sh-eoe-indicator=3D - =3Dorg-babel-sh-eoe-output=3D - =3Dorg-babel--variable-assignments:sh-generic=3D - =3Dorg-babel-sh-var-to-sh=3D - =3Dorg-babel-sh-var-to-string=3D - =3Dorg-babel-sh-initiate-session=3D - =3Dorg-babel-sh-evaluate=3D - =3Dorg-babel-sh-strip-weird-long-prompt=3D Generally speaking, I find the Org Babel code base tricky to read (especial= ly at first). I spent a good deal of time untangling what lived where and = who did what. I can play along fine now that I'm familiar. However, since u= nderstanding took longer than I think was necessary, I want to detail the p= ain points as they have made contributing to Babel harder. Overall, Babel somewhat breaks the (elisp) Coding Conventions for naming, #+begin_quote You should choose a short word to distinguish your program from other Lisp = programs. #+end_quote I understand the variable/function name prefix to be the file name, typical= ly. The file name is often the package name, or more precisely the feature= provided by the file[fn:3]. For Org Babel, there's not a solid file-to-pre= fix relation. We say "Org Babel", but the main functionality is in ob-core= and the various "ob-" files either extend or implement implied behavior (e= .g. =3Dorg-babel--execute=3D). Is the "program" ob-core, ob-lang, or= the whole suite of files? This is a subjective question which the Org Bab= el "program" answers with, "the whole suite of files". All components, acr= oss all "ob-" files, bear the name "org-babel-". This is still something th= at trips me up: is the current symbol core or not? Who is responsible for = what? I would expect the core API to have its own prefix. The extensions would th= en define their code and have a different prefix, "ob--". This way, r= eaders/contributors could open the pertinent ob-* file, see the expected sy= mbol prefix (e.g. "ob-shell-") and another prefix (e.g. "org-babel-") and b= e able to distinguish which is which. As it stands, ob-core.el could be re= named to org-babel.el or the "org-babel-" prefix could be changed to "ob-co= re-". Another possible solution, or a stopgap, would be to have a document detail= ing the Org Babel API[fn:4]. * Process interaction Emacs has several different ways of interacting with processes. The ob-shel= l.el code uses a few of them. Since async is another way to interact with a= process, a single process pattern could be used. The goal would be to make= each of the different functionalities provided by ob-shell.el have a simil= ar implementation. The expectation is that this would benefit maintenance. * Footnotes [fn:1] https://orgmode.org/worg/org-contrib/babel/languages/ob-doc-shell.ht= ml#orgfa6b7c5 [fn:2] M-up is bound to =3Dorg-metaup-hook=3D and =3Dob-core:org-babel-load-in-session=3D by default. [fn:3] It's not clear to me if there's a technical definition for an Emacs package. [fn:4] I may extend my personal notes into a document detailing the Org API. http://excalamus.com/2021-11-03-org_babel.html