From: Zelphir Kaltstahl <zelphirkaltstahl@posteo.de>
To: Ihor Radchenko <yantar92@posteo.net>
Cc: Bruno Barbier <brubar.cs@gmail.com>,
emacs-orgmode@gnu.org, Bastien <bzg@gnu.org>
Subject: Re: [PATCH] lisp/ob-scheme.el
Date: Sat, 25 Mar 2023 14:34:09 +0000 [thread overview]
Message-ID: <3208da46-d61e-98b6-fbbe-85d3e6b77291@posteo.de> (raw)
In-Reply-To: <87edphoyls.fsf@localhost>
[-- Attachment #1.1: Type: text/plain, Size: 6020 bytes --]
>> 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<zelphirkaltstahl@posteo.de>
>> 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 have to discard all those tab -> spaces
changes again, before I can make a patch. Or I have to edit in fundamental mode,
which loses syntax highlighting and is what I have been doing. But even in
fundamental-mode Emacs changes some of the indentation from tabs to spaces or
spaces to tabs apparently. Maybe I should be editing with emacs -Q or something?
But then I don't have magit. This creates quite some friction in the editing
process for me. In fundamental mode, I have to discard 4-6 chunks, which I did
not even touch, but which were changed in terms of what the indentation consists of.
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.
Attached: The updated patch.
And of course: Let me know if it needs more changes.
Regards,
Zelphir
--
repositories:https://notabug.org/ZelphirKaltstahl
[-- Attachment #1.2: Type: text/html, Size: 8431 bytes --]
[-- Attachment #2: 0001-org-babel-expand-body-scheme-define-header-arg-vars-.patch --]
[-- Type: text/x-patch, Size: 2263 bytes --]
From 99a38e67a16c7541ce2d5da2d8d46e5914e559ae Mon Sep 17 00:00:00 2001
From: Zelphir Kaltstahl <zelphirkaltstahl@posteo.de>
Date: Sat, 25 Mar 2023 15:21:38 +0100
Subject: [PATCH] org-babel-expand-body:scheme: define header arg vars using
define
* ob-scheme.el (org-babel-expand-body:scheme,
org-babel-expand-header-arg-vars:scheme): Change the way header
argument :var variables are expanded for for Scheme source blocks. Use
`define' instead of wrapping using `let'.
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.
For more context see the mailing list discussion at: https://lists.gnu.org/archive/html/emacs-orgmode/2023-03/msg00087.html
---
lisp/ob-scheme.el | 16 +++++++++-------
1 file changed, 9 insertions(+), 7 deletions(-)
diff --git a/lisp/ob-scheme.el b/lisp/ob-scheme.el
index 9f12f42cb..7424ab652 100644
--- a/lisp/ob-scheme.el
+++ b/lisp/ob-scheme.el
@@ -79,6 +79,14 @@
(defvar org-babel-default-header-args:scheme '()
"Default header arguments for scheme code blocks.")
+(defun org-babel-scheme-expand-header-arg-vars (vars)
+ "Expand :var header arguments given as VARS."
+ (mapconcat
+ (lambda (var)
+ (format "(define %S %S)" (car var) (cdr var)))
+ vars
+ "\n"))
+
(defun org-babel-expand-body:scheme (body params)
"Expand BODY according to PARAMS, return the expanded body."
(let ((vars (org-babel--get-vars params))
@@ -86,13 +94,7 @@
(postpends (cdr (assq :epilogue params))))
(concat (and prepends (concat prepends "\n"))
(if (null vars) body
- (format "(let (%s)\n%s\n)"
- (mapconcat
- (lambda (var)
- (format "%S" (print `(,(car var) ',(cdr var)))))
- vars
- "\n ")
- body))
+ (concat (org-babel-scheme-expand-header-arg-vars vars) body))
(and postpends (concat "\n" postpends)))))
--
2.25.1
next prev parent reply other threads:[~2023-03-25 14:35 UTC|newest]
Thread overview: 30+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-03-07 11:27 org-babel guile source block bug in handling multiple values Zelphir Kaltstahl
2023-03-07 14:36 ` Ihor Radchenko
2023-03-07 15:18 ` Zelphir Kaltstahl
2023-03-07 19:52 ` Bruno Barbier
2023-03-08 0:55 ` Zelphir Kaltstahl
2023-03-08 19:38 ` Bruno Barbier
2023-03-09 0:44 ` Zelphir Kaltstahl
2023-03-09 13:04 ` [BUG] Inconsistent global/local :var assignments in ob-* for lisps and non-lisps (was: org-babel guile source block bug in handling multiple values) Ihor Radchenko
2023-03-10 10:39 ` Zelphir Kaltstahl
2023-03-11 9:58 ` Ihor Radchenko
2023-03-11 18:30 ` Zelphir Kaltstahl
2023-03-12 11:33 ` Ihor Radchenko
2023-03-19 13:50 ` [PATCH] lisp/ob-scheme.el Zelphir Kaltstahl
2023-03-22 10:43 ` Ihor Radchenko
2023-03-25 14:34 ` Zelphir Kaltstahl [this message]
2023-03-26 9:32 ` Ihor Radchenko
2023-04-25 12:28 ` Ihor Radchenko
2023-04-29 11:08 ` Zelphir Kaltstahl
2023-03-09 13:10 ` org-babel guile source block bug in handling multiple values Ihor Radchenko
2023-03-10 10:42 ` Zelphir Kaltstahl
2023-03-11 10:18 ` Ihor Radchenko
2023-06-02 13:11 ` Ihor Radchenko
2023-03-09 13:11 ` Ihor Radchenko
2023-03-09 14:21 ` Daniel Kraus
2023-03-10 11:57 ` Ihor Radchenko
2023-03-10 10:45 ` Zelphir Kaltstahl
2023-03-08 1:13 ` Zelphir Kaltstahl
2023-03-08 8:55 ` Ihor Radchenko
2023-03-07 15:44 ` Max Nikulin
2023-03-07 21:41 ` Rudolf Adamkovič
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: https://www.orgmode.org/
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=3208da46-d61e-98b6-fbbe-85d3e6b77291@posteo.de \
--to=zelphirkaltstahl@posteo.de \
--cc=brubar.cs@gmail.com \
--cc=bzg@gnu.org \
--cc=emacs-orgmode@gnu.org \
--cc=yantar92@posteo.net \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://git.savannah.gnu.org/cgit/emacs/org-mode.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).