* [PATCH] ob-shell: consistent prefix
@ 2024-01-15 21:12 Matt
2024-01-16 13:12 ` Ihor Radchenko
0 siblings, 1 reply; 3+ messages in thread
From: Matt @ 2024-01-15 21:12 UTC (permalink / raw)
To: emacs-orgmode
[-- Attachment #1: Type: text/plain, Size: 1924 bytes --]
The attached patch makes names in ob-shell.el use the same prefix
"org-babel-shell-" (along with a few similar minor consistency
updates).
Without the patch, three naming conventions exist:
- org-babel-shell
- org-babel-sh
- ob-shell <----- that's my fault :)
The Org Babel shell library was previously called "ob-sh.el" and later
changed to "ob-shell.el". When this happened, the meaning of the "sh"
changed. When the library was called "ob-sh.el", "sh" meant a generic
shell. When the library was renamed "ob-shell.el", "shell" took on
the role of a generic shell and "sh" became a reference to /bin/sh.
The current "sh" names are generic and not specific to "/bin/sh" or
something like the Bourne Shell. The patch updates all "sh" names
to "shell".
When I submitted my async changes, I introduced an "ob-shell" prefix.
Emacs names typically begin with the library name and I just continued
in that style. Org is different, however, because it's the "org-"
library. For sub-libraries, like Babel, the convention is
"org-babel-<language>-". ob-shell.el is unusual in that it supports
multiple languages. However, ob-C.el also supports multiple languages
and uses "org-babel-<language>-". I find there's no confusion using
this style.
FWIW, I found it confusing when I first started learning the Org code
base that the naming convention is at the "org-" level and not at the
library level (for example, "ob-shell-"). A large part of my
confusion, I think, was simply inconsistencies within ob-shell.
It's late for me, so rather than commit in a rush, I'm posting this
for review/comment. The tests pass and I think I preserved all the
API function names (like org-babel-execute:template).
Thoughts?
--
Matt Trzcinski
Emacs Org contributor (ob-shell)
Learn more about Org mode at https://orgmode.org
Support Org development at https://liberapay.com/org-mode
[-- Attachment #2: 0001-make-name-prefixes-consistent.diff --]
[-- Type: application/octet-stream, Size: 11817 bytes --]
diff --git a/lisp/ob-shell.el b/lisp/ob-shell.el
index 31135b5fb..21d44391a 100644
--- a/lisp/ob-shell.el
+++ b/lisp/ob-shell.el
@@ -80,7 +80,7 @@ is modified outside the Customize interface."
(org-babel-execute:shell body params))))
(put fname 'definition-name 'org-babel-shell-initialize))
(defalias (intern (concat "org-babel-variable-assignments:" name))
- #'org-babel-variable-assignments:shell
+ #'org-babel-shell-variable-assignments:shell
(format "Return list of %s statements assigning to the block's \
variables."
name))
@@ -115,10 +115,10 @@ a shell execution being its exit code."
(defun org-babel-execute:shell (body params)
"Execute Shell BODY according to PARAMS.
This function is called by `org-babel-execute-src-block'."
- (let* ((session (org-babel-sh-initiate-session
+ (let* ((session (org-babel-shell-initiate-session
(cdr (assq :session params))))
(stdin (let ((stdin (cdr (assq :stdin params))))
- (when stdin (org-babel-sh-var-to-string
+ (when stdin (org-babel-shell-var-to-string
(org-babel-ref-resolve stdin)))))
(results-params (cdr (assq :result-params params)))
(value-is-exit-status
@@ -129,10 +129,10 @@ This function is called by `org-babel-execute-src-block'."
(cmdline (cdr (assq :cmdline params)))
(full-body (concat
(org-babel-expand-body:generic
- body params (org-babel-variable-assignments:shell params))
+ body params (org-babel-shell-variable-assignments:shell params))
(when value-is-exit-status "\necho $?"))))
(org-babel-reassemble-table
- (org-babel-sh-evaluate session full-body params stdin cmdline)
+ (org-babel-shell-evaluate session full-body params stdin cmdline)
(org-babel-pick-name
(cdr (assq :colname-names params)) (cdr (assq :colnames params)))
(org-babel-pick-name
@@ -140,8 +140,8 @@ This function is called by `org-babel-execute-src-block'."
(defun org-babel-prep-session:shell (session params)
"Prepare SESSION according to the header arguments specified in PARAMS."
- (let* ((session (org-babel-sh-initiate-session session))
- (var-lines (org-babel-variable-assignments:shell params)))
+ (let* ((session (org-babel-shell-initiate-session session))
+ (var-lines (org-babel-shell-variable-assignments:shell params)))
(org-babel-comint-in-buffer session
(mapc (lambda (var)
(insert var) (comint-send-input nil t)
@@ -160,27 +160,27 @@ This function is called by `org-babel-execute-src-block'."
\f
;;; Helper functions
-(defun org-babel--variable-assignments:sh-generic
+(defun org-babel-shell--variable-assignments:default
(varname values &optional sep hline)
"Return a list of statements declaring the values as a generic variable."
- (format "%s=%s" varname (org-babel-sh-var-to-sh values sep hline)))
+ (format "%s=%s" varname (org-babel-shell-var-to-shell values sep hline)))
-(defun org-babel--variable-assignments:fish
+(defun org-babel-shell--variable-assignments:fish
(varname values &optional sep hline)
"Return a list of statements declaring the values as a fish variable."
- (format "set %s %s" varname (org-babel-sh-var-to-sh values sep hline)))
+ (format "set %s %s" varname (org-babel-shell-var-to-shell values sep hline)))
-(defun org-babel--variable-assignments:bash_array
+(defun org-babel-shell--variable-assignments:bash_array
(varname values &optional sep hline)
"Return a list of statements declaring the values as a bash array."
(format "unset %s\ndeclare -a %s=( %s )"
varname varname
(mapconcat
- (lambda (value) (org-babel-sh-var-to-sh value sep hline))
+ (lambda (value) (org-babel-shell-var-to-shell value sep hline))
values
" ")))
-(defun org-babel--variable-assignments:bash_assoc
+(defun org-babel-shell--variable-assignments:bash_assoc
(varname values &optional sep hline)
"Return a list of statements declaring the values as bash associative array."
(format "unset %s\ndeclare -A %s\n%s"
@@ -189,22 +189,22 @@ This function is called by `org-babel-execute-src-block'."
(lambda (items)
(format "%s[%s]=%s"
varname
- (org-babel-sh-var-to-sh (car items) sep hline)
- (org-babel-sh-var-to-sh (cdr items) sep hline)))
+ (org-babel-shell-var-to-shell (car items) sep hline)
+ (org-babel-shell-var-to-shell (cdr items) sep hline)))
values
"\n")))
-(defun org-babel--variable-assignments:bash (varname values &optional sep hline)
+(defun org-babel-shell--variable-assignments:bash (varname values &optional sep hline)
"Represent the parameters as useful Bash shell variables."
(pcase values
(`((,_ ,_ . ,_) . ,_) ;two-dimensional array
- (org-babel--variable-assignments:bash_assoc varname values sep hline))
+ (org-babel-shell--variable-assignments:bash_assoc varname values sep hline))
(`(,_ . ,_) ;simple list
- (org-babel--variable-assignments:bash_array varname values sep hline))
+ (org-babel-shell--variable-assignments:bash_array varname values sep hline))
(_ ;scalar value
- (org-babel--variable-assignments:sh-generic varname values sep hline))))
+ (org-babel-shell--variable-assignments:default varname values sep hline))))
-(defun org-babel-variable-assignments:shell (params)
+(defun org-babel-shell-variable-assignments:shell (params)
"Return list of shell statements assigning the block's variables."
(let ((sep (cdr (assq :separator params)))
(hline (when (string= "yes" (cdr (assq :hlines params)))
@@ -213,25 +213,25 @@ This function is called by `org-babel-execute-src-block'."
(mapcar
(lambda (pair)
(if (string-suffix-p "bash" shell-file-name)
- (org-babel--variable-assignments:bash
+ (org-babel-shell--variable-assignments:bash
(car pair) (cdr pair) sep hline)
(if (string-suffix-p "fish" shell-file-name)
- (org-babel--variable-assignments:fish
+ (org-babel-shell--variable-assignments:fish
(car pair) (cdr pair) sep hline)
- (org-babel--variable-assignments:sh-generic
+ (org-babel-shell--variable-assignments:default
(car pair) (cdr pair) sep hline))))
(org-babel--get-vars params))))
-(defun org-babel-sh-var-to-sh (var &optional sep hline)
+(defun org-babel-shell-var-to-shell (var &optional sep hline)
"Convert an elisp value to a shell variable.
Convert an elisp var into a string of shell commands specifying a
var of the same value."
(concat "'" (replace-regexp-in-string
"'" "'\"'\"'"
- (org-babel-sh-var-to-string var sep hline))
+ (org-babel-shell-var-to-string var sep hline))
"'"))
-(defun org-babel-sh-var-to-string (var &optional sep hline)
+(defun org-babel-shell-var-to-string (var &optional sep hline)
"Convert an elisp value to a string."
(let ((echo-var (lambda (v) (if (stringp v) v (format "%S" v)))))
(cond
@@ -242,14 +242,14 @@ var of the same value."
(mapconcat echo-var var "\n"))
(t (funcall echo-var var)))))
-(defvar org-babel-sh-eoe-indicator "echo 'org_babel_sh_eoe'"
+(defvar org-babel-shell-eoe-indicator "echo 'org_babel_sh_eoe'"
"String to indicate that evaluation has completed.")
-(defvar org-babel-sh-eoe-output "org_babel_sh_eoe"
+(defvar org-babel-shell-eoe-output "org_babel_sh_eoe"
"String to indicate that evaluation has completed.")
-(defvar org-babel-sh-prompt "org_babel_sh_prompt> "
+(defvar org-babel-shell-prompt "org_babel_sh_prompt> "
"String to set prompt in session shell.")
-(defun org-babel-sh-initiate-session (&optional session _params)
+(defun org-babel-shell-initiate-session (&optional session _params)
"Initiate a session named SESSION according to PARAMS."
(when (and session (not (string= session "none")))
(save-window-excursion
@@ -264,9 +264,9 @@ var of the same value."
(or (cdr (assoc (file-name-nondirectory shell-file-name)
org-babel-shell-set-prompt-commands))
(alist-get t org-babel-shell-set-prompt-commands))
- org-babel-sh-prompt))
+ org-babel-shell-prompt))
(setq-local comint-prompt-regexp
- (concat "^" (regexp-quote org-babel-sh-prompt)
+ (concat "^" (regexp-quote org-babel-shell-prompt)
" *"))
;; Needed for Emacs 23 since the marker is initially
;; undefined and the filter functions try to use it without
@@ -274,16 +274,16 @@ var of the same value."
(set-marker comint-last-output-start (point))
(get-buffer (current-buffer)))))))
-(defconst ob-shell-async-indicator "echo 'ob_comint_async_shell_%s_%s'"
+(defconst org-babel-shell-async-indicator "echo 'ob_comint_async_shell_%s_%s'"
"Session output delimiter template.
See `org-babel-comint-async-indicator'.")
-(defun ob-shell-async-chunk-callback (string)
+(defun org-babel-shell-async-chunk-callback (string)
"Filter applied to results before insertion.
See `org-babel-comint-async-chunk-callback'."
(replace-regexp-in-string comint-prompt-regexp "" string))
-(defun org-babel-sh-evaluate (session body &optional params stdin cmdline)
+(defun org-babel-shell-evaluate (session body &optional params stdin cmdline)
"Pass BODY to the Shell process in BUFFER.
If RESULT-TYPE equals `output' then return a list of the outputs
of the statements in BODY, if RESULT-TYPE equals `value' then
@@ -328,28 +328,28 @@ return the value of the last statement in BODY."
session
(current-buffer)
"ob_comint_async_shell_\\(.+\\)_\\(.+\\)"
- 'ob-shell-async-chunk-callback
+ 'org-babel-shell-async-chunk-callback
nil)
(org-babel-comint-async-delete-dangling-and-eval
session
- (insert (format ob-shell-async-indicator "start" uuid))
+ (insert (format org-babel-shell-async-indicator "start" uuid))
(comint-send-input nil t)
(insert (org-trim body))
(comint-send-input nil t)
- (insert (format ob-shell-async-indicator "end" uuid))
+ (insert (format org-babel-shell-async-indicator "end" uuid))
(comint-send-input nil t))
uuid))
(mapconcat
- #'org-babel-sh-strip-weird-long-prompt
+ #'org-babel-shell-strip-weird-long-prompt
(mapcar
#'org-trim
(butlast ; Remove eoe indicator
(org-babel-comint-with-output
- (session org-babel-sh-eoe-output t body)
+ (session org-babel-shell-eoe-output t body)
(insert (org-trim body) "\n"
- org-babel-sh-eoe-indicator)
+ org-babel-shell-eoe-indicator)
(comint-send-input nil t))
- ;; Remove `org-babel-sh-eoe-indicator' output line.
+ ;; Remove `org-babel-shell-eoe-indicator' output line.
1))
"\n")))
;; External shell script, with or without a predefined
@@ -380,7 +380,7 @@ return the value of the last statement in BODY."
(with-temp-file tmp-file (insert results))
(org-babel-import-elisp-from-file tmp-file)))))))
-(defun org-babel-sh-strip-weird-long-prompt (string)
+(defun org-babel-shell-strip-weird-long-prompt (string)
"Remove prompt cruft from a string of shell output."
(while (string-match "^% +[\r\n$]+ *" string)
(setq string (substring string (match-end 0))))
^ permalink raw reply related [flat|nested] 3+ messages in thread* Re: [PATCH] ob-shell: consistent prefix
2024-01-15 21:12 [PATCH] ob-shell: consistent prefix Matt
@ 2024-01-16 13:12 ` Ihor Radchenko
2024-01-20 16:03 ` Matt
0 siblings, 1 reply; 3+ messages in thread
From: Ihor Radchenko @ 2024-01-16 13:12 UTC (permalink / raw)
To: Matt; +Cc: emacs-orgmode
Matt <matt@excalamus.com> writes:
> The attached patch makes names in ob-shell.el use the same prefix
> "org-babel-shell-" (along with a few similar minor consistency
> updates).
>
> Without the patch, three naming conventions exist:
>
> - org-babel-shell
> - org-babel-sh
> - ob-shell <----- that's my fault :)
> ...
> The current "sh" names are generic and not specific to "/bin/sh" or
> something like the Bourne Shell. The patch updates all "sh" names
> to "shell".
I do not mind changing the names, except that we must not break
backwards compatibility. In particular, the non-private function and
variable names that were present in the latest Org stable release must
be either supplied with an alias or declared obsolete. Otherwise, the
third-party code using the old names will be broken.
It is ok to change the variable names when the variables are introduced
within current main branch and not yet released.
Further, some function names are not arbitrary, but are instead dictated
by the rules ob-core.el demands from the babel backends. The following
variable and function names are special and must be named specifically
by babel backends:
- org-babel-default-header-args:<lang>
- org-babel-execute:<lang>
- org-babel-expand-body:<lang>
- org-babel-variable-assignments:<lang>
- org-babel-header-args:<lang>
- org-babel-load-session:<lang>
- org-babel-<lang>-initiate-session
- org-babel-prep-session:<lang>
- org-babel-edit-prep:<lang>
- org-babel-<lang>-associate-session
> -(defun org-babel-variable-assignments:shell (params)
> +(defun org-babel-shell-variable-assignments:shell (params)
This will break ob-shell as we change the expected function name from
the above list.
> -(defun org-babel-sh-initiate-session (&optional session _params)
> +(defun org-babel-shell-initiate-session (&optional session _params)
In theory, this should still work because `org-babel-shell-initialize'
create aliases to define multiple babel backends, for each shell name in
`org-babel-shell-names'. However, no alias is created for
`org-babel-<lang>-initiate-session' in particular, which is probably a
bug.
--
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>
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] ob-shell: consistent prefix
2024-01-16 13:12 ` Ihor Radchenko
@ 2024-01-20 16:03 ` Matt
0 siblings, 0 replies; 3+ messages in thread
From: Matt @ 2024-01-20 16:03 UTC (permalink / raw)
To: Ihor Radchenko; +Cc: emacs-orgmode
I'm going to defer these changes for now. It seems like it would be easier to handle renaming one function/variable at a time. Renaming requires creating an alias mapping between the old name and the new one. Any subsequent refactoring would require updating the mapping. It seems better to do any non-naming refactoring and then handle aliasing and obsoleting in order to minimize the number of changes that need to happen. Also, renaming now invalidates any references, like in my notes or documentation.
I have questions regarding development that I'll ask in a different subject thread.
--
Matt Trzcinski
Emacs Org contributor (ob-shell)
Learn more about Org mode at https://orgmode.org
Support Org development at https://liberapay.com/org-mode
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2024-01-20 16:03 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-01-15 21:12 [PATCH] ob-shell: consistent prefix Matt
2024-01-16 13:12 ` Ihor Radchenko
2024-01-20 16:03 ` Matt
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).