Re: Refactor org-babel-shell-initialize? (was Re: ob-shell intentions and paperwork (was Bash results broken?))

[Ihor Radchenko <yantar92@posteo.net>]

Matt <matt@excalamus.com> writes:
>  ---- On Sat, 31 Dec 2022 07:56:10 -0500  Ihor Radchenko  wrote --- 
>  > As for being a macro, there will be not much gain - the convention is
>  > mostly designed for things like `cl-defun' aimed to be used in the code.
>  > `org-babel-shell-initialize' is only used by `org-babel-shell-names'.
>
> I'm not sure what you mean by "to be used in code".  Do you mean called within the global scope?

There are certain conventions about indentation of macros with "defun" in
them. They are automatically applied in emacs-lisp-mode.

Also, some heuristics for imenu looks for "defun" top-level forms, AFAIR.

In all these scenarios, it is assumed that "defun" macros are used in
the source code to define functions during compile time. Not on runtime.

> 1. The source is not explicit for a given `org-babel-execute:some-shell', making it difficult to debug
>
> The benefit of using a macro is clarity of the defined functions.  Here's the core `org-babel-shell-initialize' functionality as a macro.  Note that it doesn't loop through `org-babel-shell-names'.
> ...
> You can expand to see the definitions:
>
> (pp-macroexpand-expression '(define-babel-shell-1 "csh"))
>
> Is there a way to see the definition of`org-babel-execute:csh' using the current `org-babel-shell-initialize', that is, when generated by a function?

https://github.com/Wilfred/helpful displays the function code in such
scenario. Probably, I need to raise this problem on emacs-devel.

> Looking at the expansion, I see what appears to be an error:
>
> (alist-get "csh" org-babel-shell-set-prompt-commands)
>
> `org-babel-shell-set-prompt-commands' is an alist keyed by string shell names and whose values are shell commands to set the prompt.  `alist-get' uses `eq' by default which always returns nil for string comparisons.  That is, (alist-get "csh" org-babel-shell-set-prompt-commands) returns nil, not because the corresponding alist value is nil.  Rather, because the (eq "csh" "csh") comparison evaluates as nil.  The TESTFN probably should be `equal' or `string=':
>
> (alist-get "csh" org-babel-shell-set-prompt-commands nil nil 'equal)
>
> Then, it will return the prompt associated with "csh".

Good point. Would you mind making a patch?

> 2. Source navigation gets confused when looking up help for a generated function.  For example, `C-h f org-babel-execute:sh' goes to the top of ob-shell.el

This is indeed to be expected.

> Generating the execute functions with a macro also has this problem.  I'm not sure there's a way around it other than defining each function with `defun'.  Doing that would be a bad idea because of the massive code duplication/maintenance it would create.

Yup.

> 3. It does not adhere to the coding convention.
>
> I'll requote the convention here for convenience.
>
>> (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."
>
> It's not clear to me why the convention exists, beyond consistency (a valid reason).  I suspected it was to make the code clearer (points 1) and to "help various tools find the definition automatically" (point 2).  
>
> I'm biased by my experience into thinking a macro actually addresses point 1.  I could be wrong about it, though. It could just have been luck and personal preference, and I may be overlooking some hidden complexity, a common problem with macros.  Is there anything you see that I might be overlooking?

Nothing substantial, AFAIK.

> AFAICT, a macro doesn't help with finding the definition of the generated function.  Any idea what tools it's talking about?

See above.

> Also, the way I defined `define-babel-shell-1' doesn't fit the convention.  Something like this would:
>
> (defmacro define-babel-execute-shell-2 (name)
>   "Define functions and variables needed by Org Babel to execute a shell.
>
> NAME is a symbol of the form `org-babel-execute:my-shell'."
>   (declare (indent nil) (debug t))
>   (let ((shell-program (cadr (split-string (symbol-name name) ":"))))

Symbol argument will create awkward back-and-forth conversion between
string and a symbol here.

> 4. Except for using the customize interface, changing `org-babel-shell-names' requires reevaluating the function generator (`org-babel-shell-initialize' or some variant of `define-babel-shell-1' )
>
> A macro would not solve the need to re-evaluate the function generation when a change is made to `org-babel-shell-names'.  Either way, we need to loop/map over the list of shells.
>
> I'm curious your thoughts about removing the `:set' function from `org-babel-shell-names' and using `add-variable-watcher' instead.  In my explorations, the watcher gets called when using customize, as well as when a new shell is added to `org-babel-shell-names' using `add-to-list'.

I have never seen using variable watcher for this purpose.
I suggest asking on emacs-devel first to hear what they think of it.

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