Re: ob-shell intentions and paperwork (was Bash results broken?)

[Ihor Radchenko]

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 --- 
>
>  > Currently, though, I'm refactoring the ob-shell tests to remove dependency 
> on ob-shell-test.org and to stop the suite from littering. 
>
> 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 writing 
> more code. 

Sure. See below.

> I just got the signed paperwork back from Craig and am waiting to be admitted 
> 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–it evaluates as a symbol.  However, `org-babel-execute:sh' 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.

-- 
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>