emacs-orgmode@gnu.org archives
 help / color / mirror / code / Atom feed
From: Matt <matt@excalamus.com>
To: "Ihor Radchenko" <yantar92@posteo.net>
Cc: "emacs-orgmode" <emacs-orgmode@gnu.org>
Subject: Refactor org-babel-shell-initialize? (was Re: ob-shell intentions and paperwork (was Bash results broken?))
Date: Sun, 01 Jan 2023 23:40:00 -0500	[thread overview]
Message-ID: <18570c77eb9.119e6407e40169.7047396916380100893@excalamus.com> (raw)
In-Reply-To: <87r0wfafzu.fsf@localhost>


 ---- On Sat, 31 Dec 2022 07:56:10 -0500  Ihor Radchenko  wrote --- 
 > As for being a macro, there will be not much gain - the convention is
 > mostly designed for things like `cl-defun' aimed to be used in the code.
 > `org-babel-shell-initialize' is only used by `org-babel-shell-names'.

I'm not sure what you mean by "to be used in code".  Do you mean called within the global scope?

 > I do not have objections if it were a macro though. (But I do not see
 > how it would help debugging).
 >
 > > Because `org-babel-shell-initialize' is a function factory, you can't easily examine or modify their definitions.  `C-h f org-babel-execute:sh' jumps to the top of lisp/ob-shell.el.  Changing the definition requires reevaluating the definition for all the execute functions (or first changing `org-babel-shell-names').
 > 
 > This is indeed a downside. Any better ideas?
 > ob-core dictates that we must have org-babel-execute:lang functions to
 > make things work.

My apologies, I feel there are too many separate issues I've brought up.  Since I've already brought them up, let me try to be more clear about them.  

I observe four issues with the current form of `org-babel-shell-initialize':

1. The source is not explicit for a given `org-babel-execute:some-shell', making it difficult to debug
2. Source navigation gets confused when looking up help for a generated function.  For example, `C-h f org-babel-execute:sh' goes to the top of ob-shell.el
3. It does not adhere to the coding convention
4. Except for using the customize interface, changing `org-babel-shell-names' requires reevaluating the function generator, currently `org-babel-shell-initialize'

Here's my perspective on each point.

1. The source is not explicit for a given `org-babel-execute:some-shell', making it difficult to debug

The benefit of using a macro is clarity of the defined functions.  Here's the core `org-babel-shell-initialize' functionality as a macro.  Note that it doesn't loop through `org-babel-shell-names'.

(defmacro define-babel-shell-1 (shell-name)
  (declare (indent nil) (debug t))
  (let ((function-name (intern (concat "my-org-babel-execute:" shell-name))))
    `(progn
       (defun ,function-name (body params)
         ,(format "Execute a block of %s commands with Babel." shell-name)
         (let ((shell-file-name ,shell-name)
               (org-babel-prompt-command
                (or (alist-get ,shell-name org-babel-shell-set-prompt-commands)
                    (alist-get t org-babel-shell-set-prompt-commands))))
           (org-babel-execute:shell body params)))
       (defalias
         ',(intern (concat "org-babel-variable-assignments:" shell-name))
         'org-babel-variable-assignments:shell
         ,(format "Return list of %s statements assigning to the block's variables." shell-name))
       (defvar ,(intern (concat "org-babel-default-header-args:" shell-name)) nil))))

You can expand to see the definitions:

(pp-macroexpand-expression '(define-babel-shell-1 "csh"))

Is there a way to see the definition of`org-babel-execute:csh' using the current `org-babel-shell-initialize', that is, when generated by a function?

Looking at the expansion, I see what appears to be an error:

(alist-get "csh" org-babel-shell-set-prompt-commands)

`org-babel-shell-set-prompt-commands' is an alist keyed by string shell names and whose values are shell commands to set the prompt.  `alist-get' uses `eq' by default which always returns nil for string comparisons.  That is, (alist-get "csh" org-babel-shell-set-prompt-commands) returns nil, not because the corresponding alist value is nil.  Rather, because the (eq "csh" "csh") comparison evaluates as nil.  The TESTFN probably should be `equal' or `string=':

(alist-get "csh" org-babel-shell-set-prompt-commands nil nil 'equal)

Then, it will return the prompt associated with "csh".

All this is visible from the function version of `org-babel-shell-initialize', of course.  However, it requires mentally tracking that ,name resolves to a string.  Using a macro along with `pp-macroexpand-expression' makes the defined function explicit.  The forms generated by the macro expansion are readily available to eval whereas the function version makes the definitions inaccessible (AFAICT).  

2. Source navigation gets confused when looking up help for a generated function.  For example, `C-h f org-babel-execute:sh' goes to the top of ob-shell.el

Generating the execute functions with a macro also has this problem.  I'm not sure there's a way around it other than defining each function with `defun'.  Doing that would be a bad idea because of the massive code duplication/maintenance it would create.

3. It does not adhere to the coding convention.

I'll requote the convention here for convenience.

> (elisp) Coding Conventions says,
>
> "• Constructs that define a function or variable should be macros, not
> functions, and their names should start with ‘define-’. The macro
> should receive the name to be defined as the first argument. That
> will help various tools find the definition automatically. Avoid
> constructing the names in the macro itself, since that would
> confuse these tools."

It's not clear to me why the convention exists, beyond consistency (a valid reason).  I suspected it was to make the code clearer (points 1) and to "help various tools find the definition automatically" (point 2).  

I'm biased by my experience into thinking a macro actually addresses point 1.  I could be wrong about it, though. It could just have been luck and personal preference, and I may be overlooking some hidden complexity, a common problem with macros.  Is there anything you see that I might be overlooking?

AFAICT, a macro doesn't help with finding the definition of the generated function.  Any idea what tools it's talking about?

Also, the way I defined `define-babel-shell-1' doesn't fit the convention.  Something like this would:

(defmacro define-babel-execute-shell-2 (name)
  "Define functions and variables needed by Org Babel to execute a shell.

NAME is a symbol of the form `org-babel-execute:my-shell'."
  (declare (indent nil) (debug t))
  (let ((shell-program (cadr (split-string (symbol-name name) ":"))))
    `(progn
       (defun ,name (body params)
         ,(format "Execute a block of %s commands with Babel." shell-program)
         (let ((shell-file-name ,shell-program)
               (org-babel-prompt-command
                (or (alist-get ,shell-program org-babel-shell-set-prompt-commands)
                    (alist-get t org-babel-shell-set-prompt-commands))))
           (org-babel-execute:shell body params)))
       (defalias
         ',(intern (concat "org-babel-variable-assignments:" shell-program))
         'org-babel-variable-assignments:shell
         ,(format "Return list of %s statements assigning to the block's variables." shell-program))
       (defvar ,(intern (concat "org-babel-default-header-args:" shell-program)) nil))))

AFAICT, adhering strictly to the convention would make things needlessly complicated.  The execute function's symbols would need to be interned beforehand, creating an extra step between the `org-babel-shell-names' and the execute functions, only for them to be converted and parsed out in the macro:

(intern "org-babel-execute:test")
(symbolp 'org-babel-execute:test)
(pp-macroexpand-expression '(define-babel-execute-shell-2 org-babel-execute:test))

4. Except for using the customize interface, changing `org-babel-shell-names' requires reevaluating the function generator (`org-babel-shell-initialize' or some variant of `define-babel-shell-1' )

A macro would not solve the need to re-evaluate the function generation when a change is made to `org-babel-shell-names'.  Either way, we need to loop/map over the list of shells.

I'm curious your thoughts about removing the `:set' function from `org-babel-shell-names' and using `add-variable-watcher' instead.  In my explorations, the watcher gets called when using customize, as well as when a new shell is added to `org-babel-shell-names' using `add-to-list'.





  reply	other threads:[~2023-01-02  4:41 UTC|newest]

Thread overview: 41+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-12-16 16:22 Bash results broken? Rudolf Adamkovič
2022-12-16 17:41 ` Ihor Radchenko
2022-12-18 11:19   ` Ihor Radchenko
2022-12-20  0:44     ` Rudolf Adamkovič
2022-12-20  3:40       ` Matt
2022-12-25 11:23         ` Ihor Radchenko
2022-12-26  2:25           ` Matt
2022-12-26  9:26             ` Ihor Radchenko
2022-12-21  6:17   ` ob-shell intentions and paperwork (was Bash results broken?) Matt
2022-12-27 20:48     ` Matt
2022-12-29 11:08       ` Ihor Radchenko
2022-12-29 14:20         ` Bastien Guerry
2022-12-30  5:34         ` Matt
2022-12-30  8:06           ` Bastien Guerry
2022-12-30 18:46           ` Matt
2022-12-31 14:31             ` Ihor Radchenko
2023-01-01 23:55               ` Matt
2023-01-02  9:47                 ` Ihor Radchenko
2023-01-02 16:40                   ` Matt
2023-01-03 10:50                     ` Ihor Radchenko
2023-01-03 13:00                       ` Matt
2023-01-05 10:31                         ` Ihor Radchenko
2023-01-05 11:21                           ` Bastien Guerry
2023-01-10  2:31                             ` Matt
2023-01-11 11:53                               ` Ihor Radchenko
2023-01-11 16:18                                 ` Matt
2023-01-11 17:02                                   ` Ihor Radchenko
2023-01-11 19:34                                     ` Matt
2023-01-12  8:26                                       ` Ihor Radchenko
2023-01-12 14:43                                     ` Max Nikulin
2023-01-13  9:36                                       ` Ihor Radchenko
2023-01-13 15:18                                         ` Matt
2023-01-13 15:23                                           ` Ihor Radchenko
2023-01-14  7:41                                             ` Max Nikulin
2023-01-14 10:35                                               ` Ihor Radchenko
2023-01-14 15:09                                                 ` cgit and merge commit Max Nikulin
2023-01-24 20:16                                                   ` Ihor Radchenko
2022-12-31 12:56           ` ob-shell intentions and paperwork (was Bash results broken?) Ihor Radchenko
2023-01-02  4:40             ` Matt [this message]
2023-01-03  9:29               ` Refactor org-babel-shell-initialize? (was Re: ob-shell intentions and paperwork (was Bash results broken?)) Ihor Radchenko
2023-01-05  8:32                 ` Ihor Radchenko

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=18570c77eb9.119e6407e40169.7047396916380100893@excalamus.com \
    --to=matt@excalamus.com \
    --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).