Re: [PATCH] update ob-scheme to latest changes in geiser package

Re: [PATCH] update ob-scheme to latest changes in geiser package

[Tim Van den Langenbergh]

[-- Attachment #1: Type: text/plain, Size: 489 bytes --]

Felipe,

I have taken the liberty of reformatting the patch you created for ob-scheme.
You can find it enclosed.

If it is fine with you and the org-mode maintainers it can be applied as-is,
particularly since it is a tiny change.
I have also added some additional information to the commit message, such as
when the `geiser-eval-region/wait' function was introduced.

Please feel free to contact me in regards to any comments or questions you may
have.

Vale,

- Tim Van den Langenbergh


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: lisp-ob-scheme.el-Use-synchronous-eval --]
[-- Type: text/x-patch, Size: 2016 bytes --]

From d06f6d5b7342693b4065c0c361e430c4bdb065e5 Mon Sep 17 00:00:00 2001
From: Tim Van den Langenbergh <tmt_vdl@gmx.com>
Date: Sat, 23 Apr 2022 14:57:27 +0200
Subject: [PATCH] lisp/ob-scheme.el: Use synchronous eval.

* ob-scheme.el (org-babel-scheme-execute-with-geiser): If available,
use the synchronous `geiser-eval-region/wait' function to get the
evaluation result.

Works with geiser 0.18 (before introduction of async evaluation) and
geiser 0.22 (which introduces `geiser-eval-region/wait').

Modified from a patch proposal by Felipe Lema.

TINYCHANGE
---
 lisp/ob-scheme.el | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/lisp/ob-scheme.el b/lisp/ob-scheme.el
index 72d2e029e..f28cadd87 100644
--- a/lisp/ob-scheme.el
+++ b/lisp/ob-scheme.el
@@ -55,6 +55,8 @@
 (declare-function geiser-mode "ext:geiser-mode" ())
 (declare-function geiser-eval-region "ext:geiser-mode"
                   (start end &optional and-go raw nomsg))
+(declare-function geiser-eval-region/wait "ext:geiser-mode"
+                  (start end &optional timeout))
 (declare-function geiser-repl-exit "ext:geiser-repl" (&optional arg))
 (declare-function geiser-eval--retort-output "ext:geiser-eval" (ret))
 (declare-function geiser-eval--retort-result-str "ext:geiser-eval" (ret prefix))
@@ -176,7 +178,11 @@ is true; otherwise returns the last value."
 	  (setq geiser-impl--implementation nil)
 	  (let ((geiser-debug-jump-to-debug-p nil)
 		(geiser-debug-show-debug-p nil))
-	    (let ((ret (geiser-eval-region (point-min) (point-max))))
+	    (let ((ret (funcall (if (fboundp 'geiser-eval-region/wait)
+                                    #'geiser-eval-region/wait
+                                  #'geiser-eval-region)
+                                (point-min)
+                                (point-max))))
 	      (setq result (if output
 			       (or (geiser-eval--retort-output ret)
 				   "Geiser Interpreter produced no output")
--
2.35.3

Re: [PATCH] update ob-scheme to latest changes in geiser package

[Felipe Lema]

I'm Ok with this patch.


Haven't tested it myself, but looks good otherwise (probably Tim has 
been using it for quite a while now)


Felipe

On 2022-04-23 09:29, Tim Van den Langenbergh wrote:
> Felipe,
>
> I have taken the liberty of reformatting the patch you created for ob-scheme.
> You can find it enclosed.
>
> If it is fine with you and the org-mode maintainers it can be applied as-is,
> particularly since it is a tiny change.
> I have also added some additional information to the commit message, such as
> when the `geiser-eval-region/wait' function was introduced.
>
> Please feel free to contact me in regards to any comments or questions you may
> have.
>
> Vale,
>
> - Tim Van den Langenbergh
>

Re: [PATCH] update ob-scheme to latest changes in geiser package

[Ihor Radchenko]

Tim Van den Langenbergh <tmt_vdl@gmx.com> writes:

> I have taken the liberty of reformatting the patch you created for ob-scheme.
> You can find it enclosed.

Thanks! Since Felipe has no objections, your patch should be good to apply.

> If it is fine with you and the org-mode maintainers it can be applied as-is,
> particularly since it is a tiny change.
> I have also added some additional information to the commit message, such as
> when the `geiser-eval-region/wait' function was introduced.

Could you kindly add the geiser version info as a comment inside code?
Commit message is an unlikely place to be checked regularly and be
noticed as geiser is updated in future.

Comments are generally preferred over commit messages:

https://orgmode.org/worg/org-contribute.html#commit-messages ->
-> https://git.savannah.gnu.org/cgit/emacs.git/plain/CONTRIBUTE
>> - Explaining the rationale for a design choice is best done in comments
>>   in the source code.  However, sometimes it is useful to describe just
>>   the rationale for a change; that can be done in the commit message
>>   between the summary line and the file entries.

Best,
Ihor

Re: [PATCH] update ob-scheme to latest changes in geiser package

[Tim Van den Langenbergh]

[-- Attachment #1: Type: text/plain, Size: 1031 bytes --]


Ihor Radchenko <yantar92@gmail.com> writes:

>
> Thanks! Since Felipe has no objections, your patch should be good to apply.
>
> Could you kindly add the geiser version info as a comment inside code?
> Commit message is an unlikely place to be checked regularly and be
> noticed as geiser is updated in future.
>
> Comments are generally preferred over commit messages:
>
> https://orgmode.org/worg/org-contribute.html#commit-messages ->
> -> https://git.savannah.gnu.org/cgit/emacs.git/plain/CONTRIBUTE
>>> - Explaining the rationale for a design choice is best done in comments
>>>   in the source code.  However, sometimes it is useful to describe just
>>>   the rationale for a change; that can be done in the commit message
>>>   between the summary line and the file entries.
>
> Best,
> Ihor

Hello Ihor,

thank you for pointing that out, seems I botched my reading comprehension skill
check.

I have updated the patch and enclosed it, feel free to drop a line if you
notice anything else obviously missing.

Vale,

- Tim


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-lisp-ob-scheme.el-Use-synchronous-eval --]
[-- Type: text/x-patch, Size: 2158 bytes --]

From 3b4120d5b3fb41d4b0a5bbdee4cc7a3f4cd02642 Mon Sep 17 00:00:00 2001
From: Tim Van den Langenbergh <tmt_vdl@gmx.com>
Date: Sat, 23 Apr 2022 14:57:27 +0200
Subject: [PATCH] lisp/ob-scheme.el: Use synchronous eval.

* ob-scheme.el (org-babel-scheme-execute-with-geiser): If available,
use the synchronous `geiser-eval-region/wait' function to get the
evaluation result.

Works with geiser 0.18 (before introduction of async evaluation) and
geiser 0.22 (which introduces `geiser-eval-region/wait').

Modified from a patch proposal by Felipe Lema.

TINYCHANGE
---
 lisp/ob-scheme.el | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/lisp/ob-scheme.el b/lisp/ob-scheme.el
index 72d2e029e..b91c368a7 100644
--- a/lisp/ob-scheme.el
+++ b/lisp/ob-scheme.el
@@ -55,6 +55,8 @@
 (declare-function geiser-mode "ext:geiser-mode" ())
 (declare-function geiser-eval-region "ext:geiser-mode"
                   (start end &optional and-go raw nomsg))
+(declare-function geiser-eval-region/wait "ext:geiser-mode"
+                  (start end &optional timeout))
 (declare-function geiser-repl-exit "ext:geiser-repl" (&optional arg))
 (declare-function geiser-eval--retort-output "ext:geiser-eval" (ret))
 (declare-function geiser-eval--retort-result-str "ext:geiser-eval" (ret prefix))
@@ -176,7 +178,13 @@ is true; otherwise returns the last value."
 	  (setq geiser-impl--implementation nil)
 	  (let ((geiser-debug-jump-to-debug-p nil)
 		(geiser-debug-show-debug-p nil))
-	    (let ((ret (geiser-eval-region (point-min) (point-max))))
+            ;; `geiser-eval-region/wait' was introduced to await the
+            ;; result of async evaluation in geiser version 0.22.
+	    (let ((ret (funcall (if (fboundp 'geiser-eval-region/wait)
+                                    #'geiser-eval-region/wait
+                                  #'geiser-eval-region)
+                                (point-min)
+                                (point-max))))
 	      (setq result (if output
 			       (or (geiser-eval--retort-output ret)
 				   "Geiser Interpreter produced no output")
--
2.36.0

Re: [PATCH] update ob-scheme to latest changes in geiser package

[Ihor Radchenko]

Tim Van den Langenbergh <tmt_vdl@gmx.com> writes:

> I have updated the patch and enclosed it, feel free to drop a line if you
> notice anything else obviously missing.

Thanks!
Applied onto main as 06d08e6f3.

Best,
Ihor

[PATCH] update ob-scheme to latest changes in geiser package

[Felipe Lema]

[-- Attachment #1: Type: text/plain, Size: 548 bytes --]

Sup, y'all

The geiser package had a recent update in which `geiser-eval-region` behaves like an async function and does not return evaluation result. In exchange, the newly introduced `geiser-eval-region/wait` does that now.

I've attached a patch to update ob-scheme.el to reflect these changes. Without them, evaluating scheme dialect code blocks in Org will end up with empty results.

To learn more about the change (and how it impacted other people's flows) see issue thread below

https://gitlab.com/emacs-geiser/geiser/-/issues/30

Felipe


[-- Attachment #2: org-babel-scheme-geiser-eval-region.wait.diff --]
[-- Type: text/x-patch, Size: 820 bytes --]

update to new API call to evaluate region and wait for result

* ob-scheme.el replace `geiser-eval-region` with `geiser-eval-region/wait` as the former changed expected behaviour; the latter returns the result of evaluating.

diff --git a/lisp/org/ob-scheme.el b/lisp/org/ob-scheme.el
index f4836b23fe..89c6abf686 100644
--- a/lisp/org/ob-scheme.el
+++ b/lisp/org/ob-scheme.el
@@ -176,7 +176,7 @@ org-babel-scheme-execute-with-geiser
 	  (setq geiser-impl--implementation nil)
 	  (let ((geiser-debug-jump-to-debug-p nil)
 		(geiser-debug-show-debug-p nil))
-	    (let ((ret (geiser-eval-region (point-min) (point-max))))
+	    (let ((ret (geiser-eval-region/wait (point-min) (point-max))))
 	      (setq result (if output
 			       (or (geiser-eval--retort-output ret)
 				   "Geiser Interpreter produced no output")

Re: [PATCH] update ob-scheme to latest changes in geiser package

[Felipe Lema]

[-- Attachment #1: Type: text/plain, Size: 794 bytes --]

On Wednesday, 29 December 2021 15.03.47 -03 Felipe Lema wrote:
> Sup, y'all
> 
> The geiser package had a recent update in which `geiser-eval-region` behaves like an async function and does not return evaluation result. In exchange, the newly introduced `geiser-eval-region/wait` does that now.
> 
> I've attached a patch to update ob-scheme.el to reflect these changes. Without them, evaluating scheme dialect code blocks in Org will end up with empty results.
> 
> To learn more about the change (and how it impacted other people's flows) see issue thread below
> 
> https://gitlab.com/emacs-geiser/geiser/-/issues/30
> 
> Felipe
> 
> 

I was pointed out that I was missing the corresponding `declare-function`.

I've included it in the attached patch. Sorry about the double-posting

Felipe

[-- Attachment #2: org-babel-scheme-geiser-eval-region.wait_v2.diff --]
[-- Type: text/x-patch, Size: 1429 bytes --]

update to new API call to evaluate region and wait for result

* ob-scheme.el replace `geiser-eval-region` with `geiser-eval-region/wait` as the former changed expected behaviour; the latter returns the result of evaluating.

diff --git a/lisp/org/ob-scheme.el b/lisp/org/ob-scheme.el
index f4836b23fe..bcceba1258 100644
--- a/lisp/org/ob-scheme.el
+++ b/lisp/org/ob-scheme.el
@@ -53,8 +53,8 @@ geiser-repl-window-allow-split

 (declare-function run-geiser "ext:geiser-repl" (impl))
 (declare-function geiser-mode "ext:geiser-mode" ())
-(declare-function geiser-eval-region "ext:geiser-mode"
-                  (start end &optional and-go raw nomsg))
+(declare-function geiser-eval-region/wait "ext:geiser-mode"
+                  (start end &optional timeout))
 (declare-function geiser-repl-exit "ext:geiser-repl" (&optional arg))
 (declare-function geiser-eval--retort-output "ext:geiser-eval" (ret))
 (declare-function geiser-eval--retort-result-str "ext:geiser-eval" (ret prefix))
@@ -176,7 +176,7 @@ org-babel-scheme-execute-with-geiser
 	  (setq geiser-impl--implementation nil)
 	  (let ((geiser-debug-jump-to-debug-p nil)
 		(geiser-debug-show-debug-p nil))
-	    (let ((ret (geiser-eval-region (point-min) (point-max))))
+	    (let ((ret (geiser-eval-region/wait (point-min) (point-max))))
 	      (setq result (if output
 			       (or (geiser-eval--retort-output ret)
 				   "Geiser Interpreter produced no output")

Re: [PATCH] update ob-scheme to latest changes in geiser package

[Max Nikulin]

On 30/12/2021 08:53, Felipe Lema wrote:
> On Wednesday, 29 December 2021 15.03.47 -03 Felipe Lema wrote:
>>
>> The geiser package had a recent update in which `geiser-eval-region` behaves like an async function and does not return evaluation result. In exchange, the newly introduced `geiser-eval-region/wait` does that now.
>>

> -	    (let ((ret (geiser-eval-region (point-min) (point-max))))
> +	    (let ((ret (geiser-eval-region/wait (point-min) (point-max))))

I am not a geiser user, so maybe I missed something. Does this change 
mean that org becomes incompatible with older geiser versions? E,g, 
debian and ubuntu have elpa-geiser system package.

I would consider testing if `geiser-eval-region/wait' is bound and 
fallback to `geiser-eval-region' otherwise.

Re: [PATCH] update ob-scheme to latest changes in geiser package

[Felipe Lema]

[-- Attachment #1: Type: text/plain, Size: 1425 bytes --]

On Thursday, 30 December 2021 22:50:20 +07 Max Nikulin wrote:
> On Wednesday, 29 December 2021 22.53.18 -03 Felipe Lema wrote:
> > On Wednesday, 29 December 2021 15.03.47 -03 Felipe Lema wrote:
> > > Sup, y'all
> > > 
> > > The geiser package had a recent update in which `geiser-eval-region` behaves like an async function and does not return evaluation result. In exchange, the newly introduced `geiser-eval-region/wait` does that now.
> > > 
> > > I've attached a patch to update ob-scheme.el to reflect these changes. Without them, evaluating scheme dialect code blocks in Org will end up with empty results.
> > > 
> > > To learn more about the change (and how it impacted other people's flows) see issue thread below
> > > 
> > > https://gitlab.com/emacs-geiser/geiser/-/issues/30
> > > 
> > > Felipe
> > > 
> > > 
> > 
> > I was pointed out that I was missing the corresponding `declare-function`.
> > 
> > I've included it in the attached patch. Sorry about the double-posting
> > 
> > Felipe
> > 
> 
> I am not a geiser user, so maybe I missed something. Does this change mean that org becomes incompatible with older geiser versions? E,g, debian and ubuntu have elpa-geiser system package.
> 
> 
> I would consider testing if `geiser-eval-region/wait' is bound and fallback to `geiser-eval-region' otherwise. 

I've wrapped an `if` to fallback to previous API before this breaking change in attached patch.

Felipe

[-- Attachment #2: org-babel-scheme-geiser-eval-region.wait_v3.diff --]
[-- Type: text/x-patch, Size: 1542 bytes --]

update to new API call to evaluate region and wait for result

* ob-scheme.el replace `geiser-eval-region` with `geiser-eval-region/wait` (only available in newer versions) as the former changed expected behaviour; the latter returns the result of evaluating.
diff --git a/lisp/org/ob-scheme.el b/lisp/org/ob-scheme.el
index f4836b23fe..ffbd62bf97 100644
--- a/lisp/org/ob-scheme.el
+++ b/lisp/org/ob-scheme.el
@@ -53,8 +53,6 @@ geiser-repl-window-allow-split

 (declare-function run-geiser "ext:geiser-repl" (impl))
 (declare-function geiser-mode "ext:geiser-mode" ())
-(declare-function geiser-eval-region "ext:geiser-mode"
-                  (start end &optional and-go raw nomsg))
 (declare-function geiser-repl-exit "ext:geiser-repl" (&optional arg))
 (declare-function geiser-eval--retort-output "ext:geiser-eval" (ret))
 (declare-function geiser-eval--retort-result-str "ext:geiser-eval" (ret prefix))
@@ -176,7 +174,14 @@ org-babel-scheme-execute-with-geiser
 	  (setq geiser-impl--implementation nil)
 	  (let ((geiser-debug-jump-to-debug-p nil)
 		(geiser-debug-show-debug-p nil))
-	    (let ((ret (geiser-eval-region (point-min) (point-max))))
+	    (let ((ret (funcall
+			;; use `geiser-eval-region/wait' only when available
+			;; in newer versions of `geiser'
+			(if (fboundp 'geiser-eval-region/wait)
+			    'geiser-eval-region/wait
+			  'geiser-eval-region)
+			(point-min)
+			(point-max))))
 	      (setq result (if output
 			       (or (geiser-eval--retort-output ret)
 				   "Geiser Interpreter produced no output")

Re: [PATCH] update ob-scheme to latest changes in geiser package

[Max Nikulin]

On 31/12/2021 02:01, Felipe Lema wrote:
> On Thursday, 30 December 2021 22:50:20 +07 Max Nikulin wrote:
>> I would consider testing if `geiser-eval-region/wait' is bound and fallback to `geiser-eval-region' otherwise.
> 
> I've wrapped an `if` to fallback to previous API before this breaking change in attached patch.

Felipe, thanks for the patch. It is tracked on https://updates.orgmode.org
Sometimes maintainers merge patches in large batches before a release, 
so it may takes months.

> --- a/lisp/org/ob-scheme.el
> +++ b/lisp/org/ob-scheme.el

Almost certainly it should be "lisp/ob-scheme.el" without "org" in the 
middle. It is better to prepare patches using "git format-patch" since 
it allows to quickly create a commit with proper message and author 
using "git am" command. See https://orgmode.org/worg/org-contribute.html 
for details.

> +			;; use `geiser-eval-region/wait' only when available
> +			;; in newer versions of `geiser'

Maybe specifying the version where new feature has been introduced would 
be helpful to maintainers to decide at which moment fallback code path 
can be removed.

I have not tested if geiser works, but a comment to 
https://gitlab.com/emacs-geiser/geiser/-/issues/30 confirms it.

It seems maintainers should check and update your status in respect to 
FSF papers on https://orgmode.org/worg/contributors.html since earlier 
you wrote that the procedure had been completed.

Re: [PATCH] update ob-scheme to latest changes in geiser package

[Ihor Radchenko]

The patch appears to solve the real issue others already experience. See
https://list.orgmode.org/20220205221049.ymkzevpc2ivlxwbc@bob-mtu/T/#t

Dear Felipe,

Could you kindly update the patch as requested by Max?

Best,
Ihor