Matt <m...@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>