Ihor Radchenko <yanta...@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.
>From 91352d03c897dc546f57f48a14847cc78e74ec85 Mon Sep 17 00:00:00 2001 From: Jack Kamm <jackk...@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