From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <emacs-orgmode-bounces+larch=yhetil.org@gnu.org>
Received: from mp11.migadu.com ([2001:41d0:2:4a6f::])
	(using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits))
	by ms5.migadu.com with LMTPS
	id qIpsGsB1rWNLsgAAbAwnHQ
	(envelope-from <emacs-orgmode-bounces+larch=yhetil.org@gnu.org>)
	for <larch@yhetil.org>; Thu, 29 Dec 2022 12:10:56 +0100
Received: from aspmx1.migadu.com ([2001:41d0:2:4a6f::])
	(using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits))
	by mp11.migadu.com with LMTPS
	id yNhsGsB1rWP92wAA9RJhRA
	(envelope-from <emacs-orgmode-bounces+larch=yhetil.org@gnu.org>)
	for <larch@yhetil.org>; Thu, 29 Dec 2022 12:10:56 +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 EFB20ECF5
	for <larch@yhetil.org>; Thu, 29 Dec 2022 12:10:55 +0100 (CET)
Received: from localhost ([::1] helo=lists1p.gnu.org)
	by lists.gnu.org with esmtp (Exim 4.90_1)
	(envelope-from <emacs-orgmode-bounces@gnu.org>)
	id 1pAqmm-00041A-3m; Thu, 29 Dec 2022 06:09:20 -0500
Received: from eggs.gnu.org ([2001:470:142:3::10])
 by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256)
 (Exim 4.90_1) (envelope-from <yantar92@posteo.net>)
 id 1pAqmi-0003wu-0R
 for emacs-orgmode@gnu.org; Thu, 29 Dec 2022 06:09:16 -0500
Received: from mout02.posteo.de ([185.67.36.66])
 by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256)
 (Exim 4.90_1) (envelope-from <yantar92@posteo.net>)
 id 1pAqmf-0008Vo-6b
 for emacs-orgmode@gnu.org; Thu, 29 Dec 2022 06:09:15 -0500
Received: from submission (posteo.de [185.67.36.169]) 
 by mout02.posteo.de (Postfix) with ESMTPS id 9A3CD2401B9
 for <emacs-orgmode@gnu.org>; Thu, 29 Dec 2022 12:09:09 +0100 (CET)
DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=posteo.net; s=2017;
 t=1672312151; bh=giy0c0y6JKD18VSdHeLIxWWzACh2uWzHQ96oX/4cqCw=;
 h=From:To:Cc:Subject:Date:From;
 b=ZcNWCM/5HPNbeOVdPfi5lGPrypfvN2cSqJ9C2jGM8MM3Hpz0BTRa3oCaKp5N73wTS
 djT7rUN+TSG9YgzswxxxJWNQdRG9vC3tGyhQX/Qd2h8nBBauezEQz8wz7J4EHhnD7g
 Hw5iJQOB0W4Us5KWxt7qpcau3x2DQmvp5YUqIhAuqQ5APRE5CRah1Y1PTRNGoeV3G/
 4N2DuZqMy59vNyIBITaEU/F3ogT7CmyfHOJK78/V0eQX07bF3WipsW7hr04En6VGq/
 uE8v44FpUKmvbsW9dNHqLbybRtDQeTTDKDsJYKDeNznkD2ZiegTBK9BSpbn0PeLf5Q
 2SGzDEmCkycZQ==
Received: from customer (localhost [127.0.0.1])
 by submission (posteo.de) with ESMTPSA id 4NjQd02HCtz6tmF;
 Thu, 29 Dec 2022 12:09:08 +0100 (CET)
From: Ihor Radchenko <yantar92@posteo.net>
To: Matt <matt@excalamus.com>, Bastien <bzg@gnu.org>
Cc: emacs-orgmode <emacs-orgmode@gnu.org>
Subject: Re: ob-shell intentions and paperwork (was Bash results broken?)
In-Reply-To: <18555580d75.d7b471f9370716.1497263973038999899@excalamus.com>
References: <m2tu1v8gj8.fsf@me.com> <878rj7s0ti.fsf@localhost>
 <1853354beb4.fce54d8d902653.6359367327256505471@excalamus.com>
 <18555580d75.d7b471f9370716.1497263973038999899@excalamus.com>
Date: Thu, 29 Dec 2022 11:08:59 +0000
Message-ID: <87edsiv54k.fsf@localhost>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: quoted-printable
Received-SPF: pass client-ip=185.67.36.66; envelope-from=yantar92@posteo.net;
 helo=mout02.posteo.de
X-Spam_score_int: -43
X-Spam_score: -4.4
X-Spam_bar: ----
X-Spam_report: (-4.4 / 5.0 requ) BAYES_00=-1.9, DKIM_SIGNED=0.1,
 DKIM_VALID=-0.1, DKIM_VALID_AU=-0.1, DKIM_VALID_EF=-0.1,
 RCVD_IN_DNSWL_MED=-2.3, RCVD_IN_MSPIKE_H2=-0.001, 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." <emacs-orgmode.gnu.org>
List-Unsubscribe: <https://lists.gnu.org/mailman/options/emacs-orgmode>,
 <mailto:emacs-orgmode-request@gnu.org?subject=unsubscribe>
List-Archive: <https://lists.gnu.org/archive/html/emacs-orgmode>
List-Post: <mailto:emacs-orgmode@gnu.org>
List-Help: <mailto:emacs-orgmode-request@gnu.org?subject=help>
List-Subscribe: <https://lists.gnu.org/mailman/listinfo/emacs-orgmode>,
 <mailto:emacs-orgmode-request@gnu.org?subject=subscribe>
Errors-To: emacs-orgmode-bounces+larch=yhetil.org@gnu.org
Sender: emacs-orgmode-bounces+larch=yhetil.org@gnu.org
X-Migadu-Country: US
X-Migadu-Flow: FLOW_IN
ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=yhetil.org;
	s=key1; t=1672312256;
	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=1AdhJ/TnKyuNIFOGi+lrfvcZJoCRl8mHsmTJOI3GzCQ=;
	b=mnzKH0/K01QNTl50twkXJXFEo7XcN/3HPuMbR46ZZtOtls6RP7UZCIfofzApCQGk/ToDuT
	wOxjjSub7A+yCsJSwjcGZeGK62BLpWoQUUQWMKU5R+cBnNRnZu3RrJKdVD7xYgdxeyW8wq
	wxArvlmun4Y/CW23xDx7cnY0M6tZivdThBjf2w/vL8pgZB1lxOGeYwOkwgXsBZ2VRyx0ZL
	V6GbEf+LI6TmjhcPvrhW+ZWQp8UYWK9W5++v6SLwLUXyiH8l0wXr/fAw8Ke1/OGQGw75kl
	bbq6oBUAG8zoJ8NmDtmeUKchDOxVTC6KhT3rLckhgckfRejbE/E1aOemd9eVLA==
ARC-Authentication-Results: i=1;
	aspmx1.migadu.com;
	dkim=pass header.d=posteo.net header.s=2017 header.b="ZcNWCM/5";
	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";
	dmarc=pass (policy=none) header.from=posteo.net
ARC-Seal: i=1; s=key1; d=yhetil.org; t=1672312256; a=rsa-sha256; cv=none;
	b=L4C+8LsEJ/95ySwfk+eDmOGL9EvAtPpHhBGIS+HswtFrme1hmOTNus706Dp5jla27il407
	NBuhZa/Y0YCv/NL5cNEE0xV/HeXoAmPtUxAwZeftZKzUprc1JqeHVwfdQitipCqP4kptWq
	TwEf4V3cFDZ/AHKxERtJkpgvNnL42TtDZcN1D2+Zm1QSfrObuvJe0Wcy87nXAz2Ud3lRy8
	p25+nid3o809Z+1YiJL5IWT16/C7SXPlaAdulXPy4updUOviY7qUbimZ7noNOaIUxD6ysg
	/Z1u0+a/75h8Gln+Rzf7dAs1n1B3kuyjSqzjywIFYMlIMxMl0aqYj66mpy2jng==
X-Spam-Score: -4.46
X-Migadu-Queue-Id: EFB20ECF5
Authentication-Results: aspmx1.migadu.com;
	dkim=pass header.d=posteo.net header.s=2017 header.b="ZcNWCM/5";
	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";
	dmarc=pass (policy=none) header.from=posteo.net
X-Migadu-Scanner: scn1.migadu.com
X-Migadu-Spam-Score: -4.46
X-TUID: 1U/zc+1cb7/Q

Bastien, could you please check Matt's copyright paperwork record in
FSF?

Matt <matt@excalamus.com> writes:

>  ---- On Wed, 21 Dec 2022 01:17:50 -0500  Matt  wrote ---=20
>
>  > Currently, though, I'm refactoring the ob-shell tests to remove depend=
ency on ob-shell-test.org and to stop the suite from littering.=20
>
> Done.  Branched off bugfix, 12e10eb0d, and refactored test-ob-shell.el.  =
See attached patches.

Better use main for development.
bugfix is for... bugfixes :)

>  >  After that, I intend to incorporate the worg examples as tests.
>
> This is probably a good place for me to pause and get feedback before wri=
ting more code.=20

Sure. See below.

> I just got the signed paperwork back from Craig and am waiting to be admi=
tted to the Emacs group on Savannah.

Does it mean that you are willing to maintain lisp/ob-shell.el?
We usually give write access to the maintainers and regular
contributors. AFAIR, you previously contributed to WORG but not to Org
core.

In order for you to get admitted to Emacs group, we will first need to
reach out to Emacs maintainers authorizing your write access.

Meanwhile, once Bastien confirms your copyright status, I can apply
patches on your behalf.

> From ecdb71afa00ca137b4faa737393cb027907a7f9f Mon Sep 17 00:00:00 2001
> From: Matt Trzcinski <matt@excalamus.com>
> Date: Tue, 20 Dec 2022 13:51:26 -0500
> Subject: [PATCH 01/20] test-ob-shell.el: Remove mixed tabs and spaces
>
> * test-ob-shell.el: Convert tabs to spaces.
>
> Change made under the premise that one or the other is better than
> both.

We generally avoid whitespace-only changes. They tend to generate merge
conflicts. We do whitespace changes alongside with other changes in the
code only - this minimizes merge conflicts.
See https://orgmode.org/list/874jvkz5k8.fsf@bzg.fr

>  (ert-deftest test-ob-shell/dont-error-on-empty-results ()
> -  "Was throwing an elisp error when shell blocks threw errors and
> -returned empty results."
> -  (should (null (org-babel-execute:sh "ls NoSuchFileOrDirectory.txt" nil=
))))
> +  (should (null (org-babel-execute:sh nil nil))))

nil BODY argument for `org-babel-execute:sh' is not supported.
Better use explicit empty body: "".

It is also a good idea to add docstrings to the tests for completeness.

> [fn:1] Session name is a string and not a variable (such as
> `session-name').  This is because `org-babel-execute:sh' produces a
> type error when the session name is defined with a variable.  The
> source of the error appears to be the `params' symbol in
> `org-babel-execute:sh'.  `params' does not evaluate as a variable as
> expected=E2=80=93it evaluates as a symbol.  However, `org-babel-execute:s=
h' is
> defined within a function factory which makes it difficult to debug.
> Hard code the test name for now and refactor it later once
> `org-babel-execute:sh` is refactored.

There is nothing wrong here. `org-babel-execute-src-block' takes care
about parameter processing making sure that :session value is always a
string.

> +;; TODO refactor into macro.  Currently violates (elisp) Coding
> +;; Conventions and is hard to debug.
>  (defun org-babel-shell-initialize ()
>    "Define execution functions associated to shell names.

Could you please elaborate? Which particular convention does it violate?
What is hard to debug?

>  (ert-deftest test-ob-shell/dont-insert-spaces-on-expanded-bodies ()
>    "Expanded shell bodies should not start with a blank line unless
> -the body of the tangled block does."
> +the strings of the tangled block does."

What does "strings of the tangled block" refer to? The previous variant
is a lot more clear for me.

> +(ert-deftest test-ob-shell/generic-uses-no-arrays ()
> +  "Test generic serialization of array into a single string."
> +  (org-test-with-temp-text
> +      (test-ob-shell-multiline-string
> +       "#+NAME: sample_array"
> +       "| one   |"
> +       "| two   |"
> +       "| three |"

Why do you need `test-ob-shell-multiline-string' here?
Can simply type-in the string directly, as the rest of tests do.

> +(require 'org-test (expand-file-name "../org-test.el"))

I am unsure here. What will happen if you run this file from
default-directory not the same with file location?

Also, the repetitive patches changing names + killing error buffer for
individual tests can be merged into a single patch.

--=20
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at <https://orgmode.org/>.
Support Org development at <https://liberapay.com/org-mode>,
or support my work at <https://liberapay.com/yantar92>