Re: [PATCH] Change default ob-python session command to match run-python

[Jack Kamm <jackkamm@gmail.com>]

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

Ihor Radchenko <yantar92@posteo.net> writes:

> Thanks!
> I have several comments.

Thanks for the feedback! I attach a followup patch in response. If all
looks good, I will squash before applying.

>> -(defcustom org-babel-python-command "python"
>> -  "Name of the command for executing Python code."
>> -  :version "24.4"
>> -  :package-version '(Org . "8.0")
>> +(defcustom org-babel-python-command nil
>> +  "Name of the command for interactive and non-interactive Python code.
>> +If set, it overrides `org-babel-python-command-session' and
>> +`org-babel-python-command-nonsession'."
>
> What about default value being 'auto? That would be more explicit.

Sure, I have updated `org-babel-python-command(-session)' to use auto as
default.

> :version tags should not be used when we use :package-version - we drop
> them as we change the defcustom that still has them.

I've dropped the :version tags for the updated variables. I can also
make a separate commit later to drop :version from other ob-python
variables like `org-babel-python-hline-to' and `org-babel-python-None-to'.

> Unless I miss something, it looks like you also allow command arguments,
> not just command name as the value. You may need to adjust the docstring
> to reflect this fact.

I've updated the docstrings to say they are for the command including
arguments.

>>  (defun org-babel-execute:python (body params)
>>    "Execute Python BODY according to PARAMS.
>>  This function is called by `org-babel-execute-src-block'."
>> -  (let* ((org-babel-python-command
>> +  (let* ((org-babel-python-command-nonsession
>> +	  (or (cdr (assq :python params))
>> +	      org-babel-python-command
>> +              org-babel-python-command-nonsession))
>> +         (org-babel-python-command-session
>>  	  (or (cdr (assq :python params))
>> -	      org-babel-python-command))
>> +	      org-babel-python-command
>> +              org-babel-python-command-session))
>
>> @@ -374,7 +407,7 @@ (defun org-babel-python-evaluate-external-process
>>  non-nil, then save graphical results to that file instead."
>>    (let ((raw
>>           (pcase result-type
>> -           (`output (org-babel-eval org-babel-python-command
>> +           (`output (org-babel-eval org-babel-python-command-nonsession
>>
>
> I am slightly concerned about
> `org-babel-python-evaluate-external-process' relying upon being called
> from `org-babel-execute:python' that let-binds
> `org-babel-python-nonsession'.
>
> Since `org-babel-python-evaluate-external-process' is a public function,
> it may also be called independently. And it will not obey
> `org-babel-python-command' then.

That's a good catch.

I've fixed this by reverting `org-babel-execute:python' to set
`org-babel-python-command' (not `org-babel-python-command-nonsession').
Then, `org-babel-python-evaluate-external-process' checks both variables
to decide what the command is.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0002-Incorporate-feedback-from-Ihor-to-be-squashed-before.patch --]
[-- Type: text/x-patch, Size: 6236 bytes --]

From 91352d03c897dc546f57f48a14847cc78e74ec85 Mon Sep 17 00:00:00 2001
From: Jack Kamm <jackkamm@gmail.com>
Date: Sat, 30 Dec 2023 20:44:46 -0800
Subject: [PATCH 2/2] Incorporate feedback from Ihor; to be squashed before
 applying

---
 lisp/ob-python.el | 67 ++++++++++++++++++++++++++---------------------
 1 file changed, 37 insertions(+), 30 deletions(-)

diff --git a/lisp/ob-python.el b/lisp/ob-python.el
index 2b17f41fe..5251c3b33 100644
--- a/lisp/ob-python.el
+++ b/lisp/ob-python.el
@@ -46,30 +46,27 @@ (defconst org-babel-header-args:python
     (python . :any))
   "Python-specific header arguments.")
 
-(defcustom org-babel-python-command nil
-  "Name of the command for interactive and non-interactive Python code.
-If set, it overrides `org-babel-python-command-session' and
-`org-babel-python-command-nonsession'."
-  :version "29.2"
+(defcustom org-babel-python-command 'auto
+  "Command (including arguments) for interactive and non-interactive Python code.
+When not `auto', it overrides `org-babel-python-command-session'
+and `org-babel-python-command-nonsession'."
   :package-version '(Org . "9.7")
   :group 'org-babel
-  :type '(choice string (const nil)))
+  :type '(choice string (const auto)))
 
-(defcustom org-babel-python-command-session nil
-  "Name of the command for starting interactive Python sessions.
-If `nil' (the default), uses the values from
+(defcustom org-babel-python-command-session 'auto
+  "Command (including arguments) for starting interactive Python sessions.
+If `auto' (the default), uses the values from
 `python-shell-interpreter' and `python-shell-interpreter-args'.
 If `org-babel-python-command' is set, then it overrides this
 option."
-  :version "29.2"
   :package-version '(Org . "9.7")
   :group 'org-babel
-  :type '(choice string (const nil)))
+  :type '(choice string (const auto)))
 
 (defcustom org-babel-python-command-nonsession "python"
-  "Name of the command for executing non-interactive Python code.
+  "Command (including arguments) for executing non-interactive Python code.
 If `org-babel-python-command' is set, then it overrides this option."
-  :version "29.2"
   :package-version '(Org . "9.7")
   :group 'org-babel
   :type 'string)
@@ -91,14 +88,9 @@ (defcustom org-babel-python-None-to 'hline
 (defun org-babel-execute:python (body params)
   "Execute Python BODY according to PARAMS.
 This function is called by `org-babel-execute-src-block'."
-  (let* ((org-babel-python-command-nonsession
+  (let* ((org-babel-python-command
 	  (or (cdr (assq :python params))
-	      org-babel-python-command
-              org-babel-python-command-nonsession))
-         (org-babel-python-command-session
-	  (or (cdr (assq :python params))
-	      org-babel-python-command
-              org-babel-python-command-session))
+	      org-babel-python-command))
 	 (session (org-babel-python-initiate-session
 		   (cdr (assq :session params))))
 	 (graphics-file (and (member "graphics" (assq :result-params params))
@@ -272,6 +264,20 @@ (defun org-babel-python--python-util-comint-end-of-output-p ()
      (buffer-substring-no-properties
       (car prompt) (cdr prompt)))))
 
+(defun org-babel-python--command (is-session)
+  "Helper function to return the Python command.
+This checks `org-babel-python-command', and then
+`org-babel-python-command-session' (if IS-SESSION) or
+`org-babel-python-command-nonsession' (if not IS-SESSION).  If
+IS-SESSION, this might return `nil', which means to use
+`python-shell-calculate-command'."
+  (or (unless (eq org-babel-python-command 'auto)
+        org-babel-python-command)
+      (if is-session
+          (unless (eq org-babel-python-command-session 'auto)
+            org-babel-python-command-session)
+        org-babel-python-command-nonsession)))
+
 (defvar-local org-babel-python--initialized nil
   "Flag used to mark that python session has been initialized.")
 (defun org-babel-python--setup-session ()
@@ -295,16 +301,17 @@ (defun org-babel-python-initiate-session-by-key (&optional session)
                           (org-babel-python-with-earmuffs session)))
            (python-shell-buffer-name
 	    (org-babel-python-without-earmuffs py-buffer))
-           (existing-session-p (comint-check-proc py-buffer)))
-      (if org-babel-python-command-session
-          (let* ((cmd-split (split-string-and-unquote
-                             org-babel-python-command-session))
+           (existing-session-p (comint-check-proc py-buffer))
+           (cmd (org-babel-python--command t)))
+      (if cmd
+          (let* ((cmd-split (split-string-and-unquote cmd))
                  (python-shell-interpreter (car cmd-split))
                  (python-shell-interpreter-args
-                  (append (cdr cmd-split)
-                          (when (member system-type
-                                        '(cygwin windows-nt ms-dos))
-                            (list "-i")))))
+                  (combine-and-quote-strings
+                   (append (cdr cmd-split)
+                           (when (member system-type
+                                         '(cygwin windows-nt ms-dos))
+                             (list "-i"))))))
             (run-python))
         (run-python))
       (with-current-buffer py-buffer
@@ -407,7 +414,7 @@ (defun org-babel-python-evaluate-external-process
 non-nil, then save graphical results to that file instead."
   (let ((raw
          (pcase result-type
-           (`output (org-babel-eval org-babel-python-command-nonsession
+           (`output (org-babel-eval (org-babel-python--command nil)
 				    (concat preamble (and preamble "\n")
                                             (if graphics-file
                                                 (format org-babel-python--output-graphics-wrapper
@@ -415,7 +422,7 @@ (defun org-babel-python-evaluate-external-process
                                               body))))
            (`value (let ((results-file (or graphics-file
 				           (org-babel-temp-file "python-"))))
-		     (org-babel-eval org-babel-python-command-nonsession
+		     (org-babel-eval (org-babel-python--command nil)
 		      (concat
 		       preamble (and preamble "\n")
 		       (format
-- 
2.43.0