Re: [PATCH] lisp/ob-scheme.el

[Zelphir Kaltstahl]

On 4/25/23 14:28, Ihor Radchenko wrote:

Zelphir Kaltstahl  writes:


Subject: [PATCH] org-babel-expand-body:scheme: define header arg vars using
  define

Applied, onto main, adding TINYCHANGE cookie and newline between
variable definitions and body.
https://git.savannah.gnu.org/cgit/emacs/org-mode.git/commit/?id=d97ba5ba5

You are also now listed as Org contributor, thanks!
https://git.sr.ht/~bzg/worg/commit/eee4cb25


Thank you for taking care of the remaining work!

Best wishes,
Zelphir

--
repositories: https://notabug.org/ZelphirKaltstahl

Re: [PATCH] lisp/ob-scheme.el

[Ihor Radchenko]

Zelphir Kaltstahl  writes:

> Subject: [PATCH] org-babel-expand-body:scheme: define header arg vars using
>  define

Applied, onto main, adding TINYCHANGE cookie and newline between
variable definitions and body.
https://git.savannah.gnu.org/cgit/emacs/org-mode.git/commit/?id=d97ba5ba5

You are also now listed as Org contributor, thanks!
https://git.sr.ht/~bzg/worg/commit/eee4cb25

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

Fixed.

Re: [PATCH] lisp/ob-scheme.el

[Ihor Radchenko]

Zelphir Kaltstahl  writes:

>>> +  "Expand :var header arguments given as VARS."
>>> +  (mapconcat
>>> +   (lambda (var)
>>> + (format "(define %s %S)" (car var) (cdr var)))
>> Is there any reason why you use %s for variable name? Previously it was
>> formatted with escapes (using %S).
>
> That was me thinking: "The name of the variable should just be itself, not 
> wrapped in double quotes, because in Scheme I cannot create a variable as 
> (define "abc" 123)". But maybe I misunderstood %s and %S. I also do not know, 
> how elisp's `print' treats its arguments. Will use 2 times %S then.

The main difference between %S and %s can be demonstrated using the
following:

(insert (format "%s" 'symbol\#w\ spaces))
(insert (format "%S" 'symbol\#w\ spaces))

Basically, if symbol name contains funny chars, reader may need to have
them escaped. %S does the escaping, while %s does not.

Of course, %S escapes according to Elisp reader, not Scheme.
I am not sure if Scheme syntax allows escapes in symbol names.
If not, there should be no real difference.

Also, `print' is the same as %S.

>> Also, previous version quoted the variable value with "'". Why didn't
>> you do it here?
>
> I am not sure I understand what you are referring to in the previous version. 
> Do 
> you mean that `print' quoted variable values with a single quote? Do you mean 
> this part of the previous code:
>
> (print `(,(car var) ',(cdr var)))
>
> ?

Yes, but upon closer look, it should not matter. Your code will work
just fine.

>>> + (concat (org-babel-expand-header-arg-vars:scheme vars) body))
>> `mapconcat' you used in `org-babel-expand-header-arg-vars:scheme' does
>> not add trailing newline, unlike done previously.

> Am I not adding a newline? I think I do?:

I meant after variable definitions.
You have
   (concat (org-babel-scheme-expand-header-arg-vars vars) body)
but string returned by `org-babel-scheme-expand-header-arg-vars' will
not have final newline after the very last define.

> I have a question or suggestion:
>
> When I save the file in Emacs, my Emacs turns all the tabs in there into 
> spaces. 
> Probably my own custom global config's choice about indentation. Could a 
> general 
> mode line thing be added to avoid that and nail down the current formatting 
> style, so that contributors only need to allow Emacs to run those settings 
> and 
> then not need to care about it? Currently the indentation style seems to be a 
> mix of tabs and spaces.

This is something about your setup overriding Org mode project defaults.
If you open org-mode/.dir-locals.el, you will see that we enable
indent-tabs-mode. If you still do not get it enabled, you probably
set indentation defaults aggressively it in your config via hooks or
something similar.

> And one more question:
>
> Does the name of the patch file matter? Git changed the name and I will 
> attach 
> it as it was created by git. Hope that's alright.

Patch file name does not matter.

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

Re: [PATCH] lisp/ob-scheme.el

[Zelphir Kaltstahl]

Not sure it meets all formalities. For example it is not clear to me, whether I
should add the "TINYCHANGE" at the bottom of my commit message.

You should, unless you have FSF copyright assignment.
I am not sure what "have FSF copyright assignment" means. I would not mind 
assigning copyright of that patch to FSF, but probably the paperwork would be 
way overblown for such a small change. I will simply add the "TINYCHANGE" then.

--
repositories:https://notabug.org/ZelphirKaltstahl
 From 51b299aa18e882681dd681acb51c9cb1b44f3b4e Mon Sep 17 00:00:00 2001
From: Zelphir Kaltstahl
Date: Sat, 18 Mar 2023 16:06:05 +0100
Subject: [PATCH] lisp/ob-scheme.el:

Please provide a short commit summary, not just the changed file.
See how we do it inhttps://git.savannah.gnu.org/cgit/emacs/org-mode.git/log/

Oh, I thought I had one. Not sure how I lost my commit message title. o.o

This is the kind of stuff, that causes me to procrastinate so much. I know it is 
not your fault and I know there need to be some format regulations in place to 
have a good manageable cooperation when working on a community project. I just 
want to explain, why I am so slow in following up and initially sending the 
patch ; ) Not blaming anyone!



Wrapping binding definitions using `let' can lead to issues with GNU
Guile and potentially other Scheme dialects. GNU Guile will only get
to the body of the let at evaluation time, not at macro expansion
time. If the let form wraps any imports of libraries that define
macros, then those imported macros are seen too late and their
corresponding forms inside the body of the let are not
expanded. Using `define' to define bindings avoids this problem, at
least in GNU Guile.

Please use double space between sentences. Also, it would be helpful to
provide a link to this thread for more context. (The aim of commit
message is a note for future contributors on the reason the change was
made).

Right! I forgot about that one while writing : ) Will do!

+(defun org-babel-expand-header-arg-vars:scheme (vars)

Please use org-babel-scheme-... function name. It is the usual
Elisp convention to prefix the functions as
library-name-inner-function-name.

The exception in org-babel is a set of special functions that must have
certain name pattern. Expanding header args is not one of those special
functions.
Ah, I was not aware of that. Took all naming inspiration from the 
org-babel-expand-body:scheme function name and thought my separate function 
ought to have :scheme suffix : ) Will fix!

+  "Expand :var header arguments given as VARS."
+  (mapconcat
+   (lambda (var)
+ (format "(define %s %S)" (car var) (cdr var)))

Is there any reason why you use %s for variable name? Previously it was
formatted with escapes (using %S).


That was me thinking: "The name of the variable should just be itself, not 
wrapped in double quotes, because in Scheme I cannot create a variable as 
(define "abc" 123)". But maybe I misunderstood %s and %S. I also do not know, 
how elisp's `print' treats its arguments. Will use 2 times %S then.


Or do you mean, that I should be using `print'? I thought using only `format' is 
simpler, so I did not bother with `print' and then `format'. If I choose the 
correct format modifiers, `print' should be unnecessary, right? Or does `print' 
do some magic behind the scenes, that is not expressable using merely format 
modifiers?



Also, previous version quoted the variable value with "'". Why didn't
you do it here?


I am not sure I understand what you are referring to in the previous version. Do 
you mean that `print' quoted variable values with a single quote? Do you mean 
this part of the previous code:


(print `(,(car var) ',(cdr var)))

?


+ (concat (org-babel-expand-header-arg-vars:scheme vars) body))

`mapconcat' you used in `org-babel-expand-header-arg-vars:scheme' does
not add trailing newline, unlike done previously.


Am I not adding a newline? I think I do?:


...

(mapconcat
   (lambda (var)
 (format "(define %S %S)" (car var) (cdr var)))
   vars
   "\n")

...


Is anything else required? Maybe 2 newlines instead? The previous version had a 
number of spaces as well, but since `define' is not creating additional indent 
like `let' should, I leave those away.


Thank you for the feedback!

I have a question or suggestion:

When I save the file in Emacs, my Emacs turns all the tabs in there into spaces. 
Probably my own custom global config's choice about indentation. Could a general 
mode line thing be added to avoid that and nail down the current formatting 
style, so that contributors only need to allow Emacs to run those settings and 
then not need to care about it? Currently the indentation style seems to be a 
mix of tabs and spaces.


(For example the GNU Guix project does a lot of formatting things in their 
files, that configure Emacs, so that the formatting will be automatically 
according to their practices.)


Otherwise the diff becomes huge and I 

Re: [PATCH] lisp/ob-scheme.el

[Ihor Radchenko]

Zelphir Kaltstahl  writes:

> I've created a patch, which I will attach to this e-mail.

Thanks!

> Not sure it meets all formalities. For example it is not clear to me, whether 
> I 
> should add the "TINYCHANGE" at the bottom of my commit message.

You should, unless you have FSF copyright assignment.

> Let me know, if this an OK patch or what else needs to be done or what format 
> is 
> wrong, if any.

See my (minor) comments below.

> -- 
> repositories: https://notabug.org/ZelphirKaltstahl
> From 51b299aa18e882681dd681acb51c9cb1b44f3b4e Mon Sep 17 00:00:00 2001
> From: Zelphir Kaltstahl 
> Date: Sat, 18 Mar 2023 16:06:05 +0100
> Subject: [PATCH] lisp/ob-scheme.el:

Please provide a short commit summary, not just the changed file.
See how we do it in https://git.savannah.gnu.org/cgit/emacs/org-mode.git/log/

> Wrapping binding definitions using `let' can lead to issues with GNU
> Guile and potentially other Scheme dialects. GNU Guile will only get
> to the body of the let at evaluation time, not at macro expansion
> time. If the let form wraps any imports of libraries that define
> macros, then those imported macros are seen too late and their
> corresponding forms inside the body of the let are not
> expanded. Using `define' to define bindings avoids this problem, at
> least in GNU Guile.

Please use double space between sentences. Also, it would be helpful to
provide a link to this thread for more context. (The aim of commit
message is a note for future contributors on the reason the change was
made).

> +(defun org-babel-expand-header-arg-vars:scheme (vars)

Please use org-babel-scheme-... function name. It is the usual
Elisp convention to prefix the functions as
library-name-inner-function-name.

The exception in org-babel is a set of special functions that must have
certain name pattern. Expanding header args is not one of those special
functions.

> +  "Expand :var header arguments given as VARS."
> +  (mapconcat
> +   (lambda (var)
> + (format "(define %s %S)" (car var) (cdr var)))

Is there any reason why you use %s for variable name? Previously it was
formatted with escapes (using %S).

Also, previous version quoted the variable value with "'". Why didn't
you do it here?

> +   (concat (org-babel-expand-header-arg-vars:scheme vars) body))

`mapconcat' you used in `org-babel-expand-header-arg-vars:scheme' does
not add trailing newline, unlike done previously.

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