handle_external_tool_process returns out/err as bytes if empty, as string if not empty
- Dominant language
- Python
- Stars
- 1.2k
- Forks
- 309
- PR merge metrics
- No merged PRs in 30d
Description
Short summary: on the cassandra-test branch, many ccmlib/node.py nodetool functions (anything that goes through handle_external_tool_process) are inconsistently returning stdout and stderr as strings if non-empty, but as bytes if empty. I'd like to update handle_external_tool_process to return strings consistently for stdout and stderr.
Details...
On branch cassandra-test, in ccmlib/node.py handle_external_tool_process returns stdout and stderr as string objects if they're non-empty, but as bytes objects if they're empty. This sets up users for one error or another unless they're careful about checking whether they got a string or a bytes before using the returned values, and it would be preferable to consistently return one type or another.
Details:
```
def handle_external_tool_process(process, cmd_args):
out, err = process.communicate()
if out and isinstance(out, bytes):
out = out.decode()
if err and isinstance(err, bytes):
err = err.decode()
rc = process.returncode
if rc != 0:
raise ToolError(cmd_args, rc, out, err)
ret = namedtuple('Subprocess_Return', 'stdout stderr rc')
return ret(stdout=out, stderr=err, rc=rc)
```
The inconsistency arises from the checks "if out" and "if err". An empty bytes object (b'') evaluates to False, so if one of these streams is empty, it doesn't get decoded, and it gets returned as an empty bytes object.
I'd like to change the conditional tests "if out" and "if err" to "if (out is not None)" and "if (err is not None)", as below:
```
def handle_external_tool_process(process, cmd_args):
out, err = process.communicate()
if (out is not None) and isinstance(out, bytes):
out = out.decode()
if (err is not None) and isinstance(err, bytes):
err = err.decode()
rc = process.returncode
if rc != 0:
raise ToolError(cmd_args, rc, out, err)
ret = namedtuple('Subprocess_Return', 'stdout stderr rc')
return ret(stdout=out, stderr=err, rc=rc)
```
I'll submit a PR for the change.
Contributor guide
No contributing guide indexed for this repository
Assessment
This issue has not been assessed yet.