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


Matt <matt@excalamus.com> writes:
>  ---- 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?

There are certain conventions about indentation of macros with "defun" in
them. They are automatically applied in emacs-lisp-mode.

Also, some heuristics for imenu looks for "defun" top-level forms, AFAIR.

In all these scenarios, it is assumed that "defun" macros are used in
the source code to define functions during compile time. Not on runtime.

> 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'.
> ...
> 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?

https://github.com/Wilfred/helpful displays the function code in such
scenario. Probably, I need to raise this problem on emacs-devel.

> 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".

Good point. Would you mind making a patch?

> 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

This is indeed to be expected.

> 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.

Yup.

> 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?

Nothing substantial, AFAIK.

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

See above.

> 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) ":"))))

Symbol argument will create awkward back-and-forth conversion between
string and a symbol here.

> 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'.

I have never seen using variable watcher for this purpose.
I suggest asking on emacs-devel first to hear what they think of it.

-- 
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at <https://orgmode.org/>.
Support Org development at <https://liberapay.com/org-mode>,
or support my work at <https://liberapay.com/yantar92>


  reply	other threads:[~2023-01-03  9:29 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             ` Refactor org-babel-shell-initialize? (was Re: ob-shell intentions and paperwork (was Bash results broken?)) Matt
2023-01-03  9:29               ` Ihor Radchenko [this message]
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=87h6x8kluc.fsf@localhost \
    --to=yantar92@posteo.net \
    --cc=emacs-orgmode@gnu.org \
    --cc=matt@excalamus.com \
    /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).