[PATCH] ob-eval.el: Refactoring `org-babel-eval' to improve readability

[PATCH] ob-eval.el: Refactoring `org-babel-eval' to improve readability

[Nick Savage]

[-- Attachment #1: Type: text/plain, Size: 958 bytes --]

Hello everyone,

I have attached a patch to refactor `org-babel-eval' and 
`org-babel--shell-command-on-region' to improve readability and to make 
local variables more consistent between functions.

This also removes two parameters from 
`org-babel--shell-command-on-region', START and END. The function was 
created as a simplified `shell-command-on-region' that does only part of 
what it does. Those two parameters were included. As far as I can tell, 
`org-babel--shell-command-on-region' is only ever called by 
`org-babel-eval', and there is never any uncertainty over the contents 
of START and END as they are only ever (point-min) and (point-max). 
Given how specialized this function is, I believe it highly unlikely 
this is called anywhere else. I searched github too and didn't find 
anywhere where where code is calling this so I felt it was safe. I'm 
happy to change it back if necessary.

Please let me know what you think.

Thanks,

Nick


[-- Attachment #2: 0001-ob-eval.el-Refactoring-org-babel-eval-to-improve-rea.patch --]
[-- Type: text/x-patch, Size: 6608 bytes --]

From ac0cdc66374f1c99aadd5f07e0c6d65eaaa158cc Mon Sep 17 00:00:00 2001
From: Nicholas Savage <nick@nicksavage.ca>
Date: Wed, 28 Apr 2021 06:15:47 -0400
Subject: [PATCH] ob-eval.el: Refactoring `org-babel-eval' to improve
 readability

* ob-eval.el (org-babel-eval): Improve documentation and rename local
variables to be consistent with `org-babel--shell-command-on-region'
* (org-babel--shell-command-on-region): Remove START and END as
parameters.
* (org-babel--shell-command-on-region): Refactored out parts of
function to `org-babel--write-temp-buffer-input-file' and `org-babel--get-shell-file-name'.

This removes two parameters from
`org-babel--shell-command-on-region'. It appears that START and END
were parameters only because shell-command-on-region has them. This
function is only called by org-babel-eval so it looks safe to remove
those parameters, since they are always (point-min) and (point-max)
and are never changed. Given the way the function works and that it
is, it is unlikely that any user code relies on it.
---
 lisp/ob-eval.el | 87 ++++++++++++++++++++++++++++---------------------
 1 file changed, 50 insertions(+), 37 deletions(-)

diff --git a/lisp/ob-eval.el b/lisp/ob-eval.el
index b0fca7bd9..cfde4f1ca 100644
--- a/lisp/ob-eval.el
+++ b/lisp/ob-eval.el
@@ -41,20 +41,22 @@
     (display-buffer buf))
   (message "Babel evaluation exited with code %S" exit-code))
 
-(defun org-babel-eval (cmd body)
-  "Run CMD on BODY.
-If CMD succeeds then return its results, otherwise display
-STDERR with `org-babel-eval-error-notify'."
-  (let ((err-buff (get-buffer-create " *Org-Babel Error*")) exit-code)
-    (with-current-buffer err-buff (erase-buffer))
+(defun org-babel-eval (command query)
+  "Run COMMAND on QUERY.
+Writes QUERY into a temp-buffer that is processed with
+org-babel--shell-command-on-region.  If COMMAND succeeds then return
+its results, otherwise display STDERR with
+`org-babel-eval-error-notify'."
+  (let ((error-buffer (get-buffer-create " *Org-Babel Error*")) exit-code)
+    (with-current-buffer error-buffer (erase-buffer))
     (with-temp-buffer
-      (insert body)
+      (insert query)
       (setq exit-code
 	    (org-babel--shell-command-on-region
-	     (point-min) (point-max) cmd err-buff))
+	     command error-buffer))
       (if (or (not (numberp exit-code)) (> exit-code 0))
 	  (progn
-	    (with-current-buffer err-buff
+	    (with-current-buffer error-buffer
 	      (org-babel-eval-error-notify exit-code (buffer-string)))
 	    (save-excursion
 	      (when (get-buffer org-babel-error-buffer-name)
@@ -71,26 +73,19 @@ STDERR with `org-babel-eval-error-notify'."
   (with-temp-buffer (insert-file-contents file)
 		    (buffer-string)))
 
-(defun org-babel--shell-command-on-region (start end command error-buffer)
+(defun org-babel--shell-command-on-region (command error-buffer)
   "Execute COMMAND in an inferior shell with region as input.
+Stripped down version of `shell-command-on-region' for internal use in
+Babel only.  This lets us work around errors in the original function
+in various versions of Emacs.  This expects the query to be run to be
+in the current temp buffer.  This is written into
+input-file.  ERROR-BUFFER is the name of the file which
+`org-babel-eval' has created to use for any error messages that are
+returned."
 
-Stripped down version of shell-command-on-region for internal use
-in Babel only.  This lets us work around errors in the original
-function in various versions of Emacs.
-"
   (let ((input-file (org-babel-temp-file "ob-input-"))
 	(error-file (if error-buffer (org-babel-temp-file "ob-error-") nil))
-	;; Unfortunately, `executable-find' does not support file name
-	;; handlers.  Therefore, we could use it in the local case
-	;; only.
-	(shell-file-name
-	 (cond ((and (not (file-remote-p default-directory))
-		     (executable-find shell-file-name))
-		shell-file-name)
-	       ((file-executable-p
-		 (concat (file-remote-p default-directory) shell-file-name))
-		shell-file-name)
-	       ("/bin/sh")))
+	(shell-file-name (org-babel--get-shell-file-name))
 	exit-status)
     ;; There is an error in `process-file' when `error-file' exists.
     ;; This is fixed in Emacs trunk as of 2012-12-21; let's use this
@@ -99,18 +94,13 @@ function in various versions of Emacs.
       (delete-file error-file))
     ;; we always call this with 'replace, remove conditional
     ;; Replace specified region with output from command.
-    (let ((swap (< start end)))
-      (goto-char start)
-      (push-mark (point) 'nomsg)
-      (write-region start end input-file)
-      (delete-region start end)
-      (setq exit-status
-	    (process-file shell-file-name input-file
-			  (if error-file
-			      (list t error-file)
-			    t)
-			  nil shell-command-switch command))
-      (when swap (exchange-point-and-mark)))
+    (org-babel--write-temp-buffer-input-file input-file)
+    (setq exit-status
+	  (process-file shell-file-name input-file
+			(if error-file
+			    (list t error-file)
+			  t)
+			nil shell-command-switch command))
 
     (when (and input-file (file-exists-p input-file)
 	       ;; bind org-babel--debug-input around the call to keep
@@ -135,6 +125,16 @@ function in various versions of Emacs.
       (delete-file error-file))
     exit-status))
 
+(defun org-babel--write-temp-buffer-input-file (input-file)
+  "Write the contents of the current temp buffer into INPUT-FILE."
+  (let ((start (point-min))
+        (end (point-max)))
+    (goto-char start)
+    (push-mark (point) 'nomsg)
+    (write-region start end input-file)
+    (delete-region start end)
+    (exchange-point-and-mark)))
+
 (defun org-babel-eval-wipe-error-buffer ()
   "Delete the contents of the Org code block error buffer.
 This buffer is named by `org-babel-error-buffer-name'."
@@ -142,6 +142,19 @@ This buffer is named by `org-babel-error-buffer-name'."
     (with-current-buffer org-babel-error-buffer-name
       (delete-region (point-min) (point-max)))))
 
+(defun org-babel--get-shell-file-name ()
+  "Return system `shell-file-name', defaulting to /bin/sh.
+Unfortunately, `executable-find' does not support file name
+handlers.  Therefore, we could use it in the local case only."
+  ;; FIXME: This is generic enough that it should probably be in emacs, not org-mode
+  (cond ((and (not (file-remote-p default-directory))
+	      (executable-find shell-file-name))
+	 shell-file-name)
+	((file-executable-p
+	  (concat (file-remote-p default-directory) shell-file-name))
+	 shell-file-name)
+	("/bin/sh")))
+
 (provide 'ob-eval)
 
 ;;; ob-eval.el ends here
-- 
2.20.1

Re: [PATCH] ob-eval.el: Refactoring `org-babel-eval' to improve readability

[Bastien]

Hi Nick,

Nick Savage <nick@nicksavage.ca> writes:

> I have attached a patch to refactor `org-babel-eval' and
> `org-babel--shell-command-on-region' to improve readability and to
> make local variables more consistent between functions.

That's useful, thanks a lot.

> This also removes two parameters from
> `org-babel--shell-command-on-region', START and END. The function was
> created as a simplified `shell-command-on-region' that does only part
> of what it does. Those two parameters were included. As far as I can
> tell, `org-babel--shell-command-on-region' is only ever called by
> `org-babel-eval', and there is never any uncertainty over the contents
> of START and END as they are only ever (point-min) and
> (point-max). Given how specialized this function is, I believe it
> highly unlikely this is called anywhere else. I searched github too
> and didn't find anywhere where where code is calling this so I felt it
> was safe. I'm happy to change it back if necessary.

Agreed - thanks for taking the time to check on the web whether this
would break anything.

Applied in master as commit c212b7dae.

Thanks,

-- 
 Bastien