Re: [PATCH] lisp/ob-screen.el: Support ~:var~ header args for babel blocks

[Max Nikulin]

On 25/02/2023 23:47, Ken Mankoff wrote:
> On 2023-02-25 at 08:05 -08, Max Nikulin wrote:
> > I believe, it is safer to define
> > `org-babel-variable-assignments:screen' some way: alias, substitution,
> > function that calls `org-babel-variable-assignments:shell'. I am
> > unsure which variant is better. I see a couple of callers for specific
> > language in ob-core.el.
>
> How's this with a defalias?

I do not mind since ob-shell.el uses it for particular shell names.

Concerning tables as variable values, it seems sh receives them as TAB 
separated text.

As to `org-babel-screen-test' perhaps the issue is additional newline 
added after random number. I have not tried stepping through the 
function in debugger though.

> diff --git a/etc/ORG-NEWS b/etc/ORG-NEWS
...
> +#+END_src
> +
> +
> +
>  * Version 9.6

It seems top level headings were separated by single empty line.

> diff --git a/lisp/ob-screen.el b/lisp/ob-screen.el
...
;; Reuse the variable assignment code from ob-shell
> +(defalias 'org-babel-variable-assignments:screen
> +  'org-babel-variable-assignments:shell)

Have you tried to compile using make? I expect a warning due to missed 
(require 'ob-shell).

I have no objections besides this couple of minor issues. I am not a 
maintainer though.

P.S. My expectations based on the package name were that ob-screen is 
intended for running commands on remote hosts. Reading the code I 
noticed a couple of issues that may be fixed when you will decide to 
touch this file next time:
- `org-babel-screen-session-socketname' does not respect 
`org-babel-screen-location'
- `string-match' is used where it may be replaced by `string-match-p' 
since match data is not used.

I noticed that ob-core besides org-babel-default-header-args:LANG uses 
org-babel-header-args:LANG. The latter is not defined in ob-screen, 
however I am unsure concerning its effect (completion?).