Re: [PATCH] Async evaluation in ob-shell

[Matt <matt@excalamus.com>]

Hi Jack and Jeremie!  I'm curious your thoughts about what Ihor and I are discussing at the end of this message about `md5'.

 ---- On Tue, 07 Mar 2023 07:45:02 -0500  Ihor Radchenko  wrote --- 
 > Matt matt@excalamus.com> writes:
 > 
 > >  > The actual prompt is "org_babel_sh_prompt> ".
 > >
 > > Agreed.
 > >
 > >  > And we want to skip leading spaces in addition.
 > >  
 > > What do you mean by this?
 > 
 > I was following the pattern described in the docstring of
 > `comint-prompt-regexp', where it is suggested that prompts should follow
 > the pattern "^ *".
 > 
 > In the case of ob-shell.el, `org-babel-sh-prompt' is a string to be used
 > as a prompt and the corresponding regexp patter will be
 > "^ *". Hence
 > 
 >   (concat "^" (regexp-quote org-babel-sh-prompt) " *")
 > 
 > >  > Adding " *" does not make the prompt match 2 spaces, but 1+.
 > >  
 > > Agreed.  
 > >
 > > It's not clear to me what pattern you're looking to match.
 > 
 > I hope the above clarified things.

I'm confused because when I run the Org from HEAD, I get:

(concat "^" (regexp-quote org-babel-sh-prompt) " *") -> "^org_babel_sh_prompt>  *"

That's *two* spaces between '>' and '*', not one.  The second space comes from either 1) the definition of `org-babel-sh-prompt', which is "org_babel_sh_prompt> " (with a single space) or 2) the " *" in the  (concat "^" (regexp-quote org-babel-sh-prompt) " *") expression.   Currently, the two combine via the concat to give *two* spaces in the regexp.

If I understand you correctly, you're trying to match the pattern given in the `comint-prompt-regexp' which is *one* space.   That's what I'm trying to do, too.

Way back in https://list.orgmode.org/87sfeyc7qr.fsf@localhost/, we had this exchange:

 ---- On Mon, 20 Feb 2023 11:24:52 +0000  Ihor Radchenko  wrote --- 
> Matt <matt@excalamus.com> writes:
>
> > +(defun ob-shell-async-chunk-callback (string)
> > +  "Filter applied to results before insertion.
> > +See `org-babel-comint-async-chunk-callback'."
> > +  (replace-regexp-in-string (concat org-babel-sh-prompt "*") "" string))
>
 > Why not using `comint-prompt-regexp'?

I switched out `org-babel-sh-prompt'  with `comint-prompt-regexp' so that the expression looks like:

+(defun ob-shell-async-chunk-callback (string)
+  "Filter applied to results before insertion.
+See `org-babel-comint-async-chunk-callback'."
+  (replace-regexp-in-string comint-prompt-regexp "" string))

This causes the new test `test-ob-shell/session-async-evaluation' to fail, as you pointed out: https://list.orgmode.org/87bkl96g6e.fsf@localhost/

The test fails when we switch out the prompt in the callback because `comint-prompt-regexp' has two spaces in it.  The second space causes a prompt to not be filtered (by the callback).  The output becomes ": 1\n: 2\n: org_babel_sh_prompt>\n" instead of  ": 1\n: 2\n" .  This looks like a bug in the `comint-prompt-regexp''.

It could be that `test-ob-shell/session-async-evaluation' doesn't test correctly, but it looks right to me (I could certainly be mistaken).  Therefore, I see only two options to fix it: remove a space from the concat expression (which I did in my latest patch) or remove a space from `org-babel-sh-prompt'.

Am I missing something?

 > >  > `md5' will be slightly faster compared to `org-id-uuid'. But it should
 > >  > not matter.
 > >  > 
 > >  > If we want use `org-id-uuid', lets move it to org-macs.el. Requiring the
 > >  > whole org-id.el must not be done. It has side effects of defining id:
 > >  > links.
 > >
 > > In the next revision (once we figure out the regex), I can create a separate commit moving `org-id-uuid' to org-macs.el and updating ob-R and ob-python from `md5' to `org-id-uuid' (assuming that's not an issue for the maintainers of those).  If you think speed is a concern, however, I can switch ob-shell.el to use plain `md5'.
 > 
 > I am in favour of using `org-id-uuid'. It might also be a useful generic
 > function for other purposes.
 > 
 > A slight concern is that some third-party code might depend on the
 > current pattern used for UUID string in ob-python. But we made no
 > promises here.
 > 
 > To be a bit safer, we can also refactor `org-uuidgen-p' exposing the
 > regexp used to match UUID. Also, it will make sense to move
 > `org-uuidgen-p' to org-macs.el.

I'm okay with all that.  I wonder, do Jack Kamm (of ob-python fame) and Jeremie Juste (of ob-R fame) have any thoughts on the matter.  I ask out of courtesy since they're the maintainers of those packages and I don't want to cross any boundaries by changing their implementations beneath them.