[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH] Async evaluation in ob-shell
From: |
Matt |
Subject: |
Re: [PATCH] Async evaluation in ob-shell |
Date: |
Thu, 09 Mar 2023 12:36:17 -0500 |
User-agent: |
Zoho Mail |
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.
- Re: [PATCH] Async evaluation in ob-shell, Matt, 2023/03/01
- Re: [PATCH] Async evaluation in ob-shell, Ihor Radchenko, 2023/03/03
- Re: [PATCH] Async evaluation in ob-shell, Matt, 2023/03/03
- Re: [PATCH] Async evaluation in ob-shell, Ihor Radchenko, 2023/03/05
- Re: [PATCH] Async evaluation in ob-shell, Matt, 2023/03/06
- Re: [PATCH] Async evaluation in ob-shell, Ihor Radchenko, 2023/03/07
- Re: [PATCH] Async evaluation in ob-shell,
Matt <=
- Re: [PATCH] Async evaluation in ob-shell, Max Nikulin, 2023/03/09
- Re: [PATCH] Async evaluation in ob-shell, Jack Kamm, 2023/03/12
- Re: [PATCH] Async evaluation in ob-shell, Ihor Radchenko, 2023/03/18
- Re: [PATCH] Async evaluation in ob-shell, Matt, 2023/03/21
- Re: [PATCH] Async evaluation in ob-shell, Ihor Radchenko, 2023/03/22
- Re: [PATCH] Async evaluation in ob-shell, Ihor Radchenko, 2023/03/23
- Re: [PATCH] Async evaluation in ob-shell, Matt, 2023/03/23
- Re: [PATCH] Async evaluation in ob-shell, Ihor Radchenko, 2023/03/24
- Re: [PATCH] Async evaluation in ob-shell, Matt, 2023/03/27
- Re: [PATCH] Async evaluation in ob-shell, Ihor Radchenko, 2023/03/28