classification
Title: Add __bool__() method to subprocess.CompletedProcess
Type: enhancement Stage: resolved
Components: Library (Lib) Versions: Python 3.7
process
Status: closed Resolution: rejected
Dependencies: Superseder:
Assigned To: Nosy List: Richard Neumann, berker.peksag, conqp, rhettinger, sayanchowdhury, serhiy.storchaka, vstinner
Priority: normal Keywords: patch

Created on 2015-10-21 10:50 by conqp, last changed 2017-02-27 11:26 by vstinner. This issue is now closed.

Files
File name Uploaded Description Edit
usecase.py conqp, 2015-10-22 16:15 Use case for subprocess.CompletedProcess.__bool__()
subprocess.patch Richard Neumann, 2016-05-12 10:44
Pull Requests
URL Status Linked Edit
PR 312 closed sayanchowdhury, 2017-02-26 14:12
Messages (14)
msg253282 - (view) Author: Richard Neumann (conqp) * Date: 2015-10-21 10:50
The class subprocess.CompletedProcess is currently lacking a __bool__() method.

It might be a practical feature to have the possibility to evaluate a CompletedProcess instance in an if/else block without the necessity to handle the exception raised by CompletedProcess.check_returncode().

Hence, I suggest adding the method

def __bool__(self):
    return self.returncode == 0

to the class.
msg253327 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2015-10-22 11:59
Can you please give examples of use cases? (code)
msg253335 - (view) Author: Richard Neumann (conqp) * Date: 2015-10-22 16:15
A useless use case is attached.
Basically it boils down to having the ability to evaluate the CompletedProcess directly by if/else rather than comparing its returncode attribute to zero each time or handling the exception raised by check_returncode().
I use this quite frequently in programs which run system commands.
Before the new subprocess.run() method and subprocess.CompletedProcess were introduced with python 3.5 I already wrote my own library for that, wich now nearly became obsoleted by the new subprocess library of python 3.5 with the expection that it does not have this feature.
See the class ProcessResult here: https://github.com/HOMEINFO/homeinfo-lib/blob/master/homeinfo/lib/system.py
msg264337 - (view) Author: Berker Peksag (berker.peksag) * (Python committer) Date: 2016-04-27 01:43
The idea sounds good to me.
msg265391 - (view) Author: Richard Neumann (Richard Neumann) Date: 2016-05-12 10:44
I took the liberty to create a patch for Python v3.5.1.
msg265393 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2016-05-12 11:03
It's a new feature, it should only go to Python 3.6.
msg265404 - (view) Author: Richard Neumann (Richard Neumann) Date: 2016-05-12 13:27
Please excuse my ambiguous phrasing.
What I meant was I created the patch _from_ the Python 3.5.1 module _for_ Python 3.6.
msg265406 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2016-05-12 14:00
This is not enough. New feature should be documented (in new method docstring, in subprocess documentation and in What's New document) and tested.

And please sign a Contributor Licensing Agreement.

http://www.python.org/psf/contrib/contrib-form/
http://www.python.org/psf/contrib/
msg265410 - (view) Author: Richard Neumann (Richard Neumann) Date: 2016-05-12 14:46
Thank you for the hint.
I never before contributed code to the python foundation and thus am not familiar with the process.
I will look into it when I find the time.
msg288075 - (view) Author: Sayan Chowdhury (sayanchowdhury) * Date: 2017-02-18 12:08
I am taking over to do the rest part required for this ticket.
msg288603 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2017-02-26 17:09
I'm not sure that it is worth to implement the __bool__() method. This doesn't add much in comparison with checking the returncode attribute.

The change can break existing code that uses boolean testing instead of checking for None.
msg288626 - (view) Author: Raymond Hettinger (rhettinger) * (Python committer) Date: 2017-02-27 09:47
> The change can break existing code that uses boolean testing 
> instead of checking for None.

For this reason, I think Guido would oppose this proposal.  In the past he has objected to iterators having a True boolean value to indicate that there were more values available for iteration.  I think similar reasoning applies here.
msg288634 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2017-02-27 11:04
See also issue26748. Classes usually have True boolean value, but empty enum classes were False-y by accident. This was considered a bug.
msg288637 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2017-02-27 11:26
For all reasons alread given in the previous comment and my comment below, I reject the proposed change.


Serhiy> See also issue26748. Classes usually have True boolean value, but empty enum classes were False-y by accident. This was considered a bug.

Another example: issue #13936, "RFE: change bool(datetime.time(0, 0, 0)) to evaluate as True". datetime.time was changed in Python 3.5 to always be true, especially for the time 00:00:00. It's the opposite of the proposed change for CompletedProcess.


The problem is also that different users can expect a different answer from bool(CompletedProcess): non-zero return code, non-empty stdout, empty stderr, etc.

CompleteProcess is a complex object with many attributes, it's not as simple as a tuple or a string, where the truthness is obvious.


> Richard Neumann: "A useless use case is attached."

"if subprocess.run(...):"

Sorry, I'm not convinced that the need of breaking the backward compatibility for this "useless use case". It's trivial to split it in two lines:

"cmd = subprocess.run(...); if cmd.returncode: (...)"

By the way, it became common that I write such code, and in this case, I need the returncode value in the if block:

"cmd = subprocess.run(...); if cmd.returncode: sys.exit(cmd.returncode)".

So I need the CompletedProcess object anyway.
History
Date User Action Args
2017-02-27 11:26:55vstinnersetstatus: open -> closed
resolution: rejected
messages: + msg288637

stage: needs patch -> resolved
2017-02-27 11:04:40serhiy.storchakasetmessages: + msg288634
2017-02-27 09:47:49rhettingersetnosy: + rhettinger
messages: + msg288626
2017-02-26 17:09:59serhiy.storchakasetmessages: + msg288603
2017-02-26 14:12:56sayanchowdhurysetpull_requests: + pull_request273
2017-02-18 12:42:37serhiy.storchakasetversions: + Python 3.7, - Python 3.6
2017-02-18 12:08:49sayanchowdhurysetnosy: + sayanchowdhury
messages: + msg288075
2016-05-12 14:46:27Richard Neumannsetmessages: + msg265410
2016-05-12 14:00:56serhiy.storchakasetnosy: + serhiy.storchaka
messages: + msg265406
2016-05-12 13:27:08Richard Neumannsetmessages: + msg265404
2016-05-12 11:03:01vstinnersetmessages: + msg265393
2016-05-12 10:44:59Richard Neumannsetfiles: + subprocess.patch

nosy: + Richard Neumann
messages: + msg265391

keywords: + patch
2016-04-27 01:43:48berker.peksagsetversions: + Python 3.6, - Python 3.5
nosy: + berker.peksag

messages: + msg264337

stage: needs patch
2015-10-22 16:15:24conqpsetfiles: + usecase.py

messages: + msg253335
2015-10-22 11:59:25vstinnersetnosy: + vstinner
messages: + msg253327
2015-10-21 10:50:58conqpcreate