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.