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

[Ihor Radchenko]

Ihor Radchenko  writes:

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

https://debbugs.gnu.org/cgi/bugreport.cgi?bug=60568

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

Fixed on bugfix.
https://git.savannah.gnu.org/cgit/emacs/org-mode.git/commit/?id=93339de71

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

https://yhetil.org/emacs-devel/87a62xiewa.fsf@localhost/T/#u

-- 
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at .
Support Org development at ,
or support my work at 

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

[Ihor Radchenko]

Matt  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 

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

[Matt]

  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?

 > I do not have objections if it were a macro though. (But I do not see
 > how it would help debugging).
 >
 > > 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 is indeed a downside. Any better ideas?
 > ob-core dictates that we must have org-babel-execute:lang functions to
 > make things work.

My apologies, I feel there are too many separate issues I've brought up.  Since 
I've already brought them up, let me try to be more clear about them.  

I observe four issues with the current form of `org-babel-shell-initialize':

1. The source is not explicit for a given `org-babel-execute:some-shell', 
making it difficult to debug
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
3. It does not adhere to the coding convention
4. Except for using the customize interface, changing `org-babel-shell-names' 
requires reevaluating the function generator, currently 
`org-babel-shell-initialize'

Here's my perspective on each point.

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

(defmacro define-babel-shell-1 (shell-name)
  (declare (indent nil) (debug t))
  (let ((function-name (intern (concat "my-org-babel-execute:" shell-name
`(progn
   (defun ,function-name (body params)
 ,(format "Execute a block of %s commands with Babel." shell-name)
 (let ((shell-file-name ,shell-name)
   (org-babel-prompt-command
(or (alist-get ,shell-name org-babel-shell-set-prompt-commands)
(alist-get t org-babel-shell-set-prompt-commands
   (org-babel-execute:shell body params)))
   (defalias
 ',(intern (concat "org-babel-variable-assignments:" shell-name))
 'org-babel-variable-assignments:shell
 ,(format "Return list of %s statements assigning to the block's 
variables." shell-name))
   (defvar ,(intern (concat "org-babel-default-header-args:" shell-name)) 
nil

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?

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

All this is visible from the function version of `org-babel-shell-initialize', 
of course.  However, it requires mentally tracking that ,name resolves to a 
string.  Using a macro along with `pp-macroexpand-expression' makes the defined 
function explicit.  The forms generated by the macro expansion are readily 
available to eval whereas the function version makes the definitions 
inaccessible (AFAICT).  

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

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.

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