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

[Matt <matt@excalamus.com>]

 ---- On Thu, 29 Dec 2022 06:08:59 -0500  Ihor Radchenko  wrote --- 
 
 > 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.

 You're correct, I've not contributed to core.  I would love to maintain lisp/ob-shell.el.  I'm expecting life changes in the coming months and can't anticipate how that will affect my time.  Would it be a problem if I need to step down as maintainer for a period?

 > > From: Matt Trzcinski matt@excalamus.com>
 > > 
 > > [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.
 
Originally the test used "yes" for a comparison string and the shell it ran in.   I changed "yes" to be the test name because when it was "yes", I didn't know what test had produced it.  However, the problem with the test name being hard coded as a string is that if the function name changes, the test string may get out of sync.  More on this below.

 > > +;; 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?

(elisp) Coding Conventions says,

"• Constructs that define a function or variable should be macros, not
     functions, and their names should start with ‘define-’.  The macro
     should receive the name to be defined as the first argument.  That
     will help various tools find the definition automatically.  Avoid
     constructing the names in the macro itself, since that would
     confuse these tools."

The `org-babel-shell-initialize' function defines *all* the `org-babel-execute:XXX' functions given by `org-babel-shell-names' (sh, bash, zsh, etc.).

Because `org-babel-shell-initialize' is a function factory, you can't easily examine or modify their definitions.  `C-h f org-babel-execute:sh' jumps to the top of lisp/ob-shell.el.  Changing the definition requires reevaluating the definition for all the execute functions (or first changing `org-babel-shell-names').

This was a problem for me when I wanted to make the session name string for `test-ob-shell/session' the test name (mentioned above).  In the test, when I replaced the session name string with a variable containing the string, `org-babel-execute:sh' failed with a type error.  I couldn't get the variable to evaluate (with backquote and comma or otherwise).  Without an explicit function definition or a macro to expand, I found it hard to debug/experiment with (and so left the test name as a hard coded string).

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

I believe this is a query-replace error. Good catch!

 > > +(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.

I probably don't need it and am happy to remove it.    An older version of the function was more complex and made sense as a separate function (or so I thought).  My aim was to make the test strings easy to read so that it's clearer what's being tested (i.e. not write multi-line strings on a single line).  I could use concat and add "\n" to the end of each line.  Or, simply write out the string-join.  Maybe there's another way to write multi-line strings that I'm not aware of in Elisp, maybe something like Python's triple-quote?

 > > +(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?

I'm unsure if this is the best approach.  AFAICT, it works.  For example,

(let ((default-directory "/home/ahab/Documents/"))
  (load "/home/ahab/Projects/org-mode/testing/lisp/test-ob-shell.el"))

The problem I'm trying to solve is that the test file relies on `org-test' yet doesn't declare it.  Instead, you need to know to load it ahead of time (or run into the error when trying to load the tests).

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

Thank you for taking the time to review everything.  I'll get a new patch set created that includes updates based on all your feedback.