[Freeipa-devel] [PATCH] 0748 Handle encoding for ipautil.run

[Jan Cholasta]

On 4.12.2015 12:43, Petr Viktorin wrote:
> On 12/04/2015 12:15 PM, Jan Cholasta wrote:
>> On 4.12.2015 10:53, Petr Viktorin wrote:
>>> On 12/04/2015 08:51 AM, Jan Cholasta wrote:
>>>> On 3.12.2015 15:42, Petr Viktorin wrote:
>>>>> On 12/03/2015 10:55 AM, Jan Cholasta wrote:
> [...]
>>>>>> If we do encode/decode in run(), I think there should be a way to
>>>>>> override the default encoding. My idea was to either accept/return
>>>>>> only
>>>>>> bytes and let the caller handle encoding themselves, or to handle
>>>>>> encoding in run(), but be able to override the defaults.
>>>>>>
>>>>>> Given we handle encoding in run(), I would imagine it like this:
>>>>>>
>>>>>>        def run(args, stdin=None, raiseonerr=True, nolog=(), env=None,
>>>>>>                capture_stdout=False, skip_output=False, cwd=None,
>>>>>>                runas=None, timeout=None, suplementary_groups=[],
>>>>>>                capture_stderr=False, encode_stdout=False,
>>>>>>                encode_stderr=False, encoding=None):
>>>>>>
>>>>>> i.e. nothing is captured by default, when stdout or stderr are
>>>>>> captured
>>>>>> they are returned as bytes by default, when stdout or stderr are
>>>>>> returned as text they are decoded using the locale encoding by
>>>>>> default,
>>>>>> or the encoding can be explicitly specified.
>>>>>>
>>>>>> Do you think this is feasible?
>>>>>
>>>>> Feasible, yes.
>>>>> In the majority of cases where the output is needed, we want text
>>>>> encoded with locale.getpreferredencoding(), so I'd rather make that the
>>>>> default rather than bytes.
>>>>>
>>>>> Or we could make "encode_stdout" imply "capture_stdout", so you
>>>>> wouldn't
>>>>> have to always specify both. (It would be better named "encoded_stdout"
>>>>> in that case.)
>>>>
>>>> I think it should be OK to decode by default. (IMO it would be best to
>>>> reverse the logic and name it "raw_stdout", with False default. Do we
>>>> need "raw_stderr" at all? I don't think we do.)
>>>
>>> Actually, now that I think about it: if the result was a namedtuple
>>> subclass, we can always set extra raw_stdout/raw_stderr attributes on
>>> it. (Unless skip_output=True, of course.)
>>>
>>>      result = run(['generate_binary_data'])
>>>      use(result.raw_stdout)
>>>
>>>      # and for backcompat,
>>>      rc, _out, _err = result
>>
>> Good idea.
>>
>> Perhaps we should use the same names as CalledProcessError for the
>> attributes, i.e. "returncode", "output", and "error_output",
>> "raw_output", "raw_error_output" for the new attributes?

(Actually, since "returncode" is not "return_code", it should probably 
be "returncode", "output", "erroroutput", "raw_output", "raw_erroroutput".)

>
> OK. It's also good to use different names than Popen's stdout/stderr,
> which are file-like objects rather than strings. But then,
> capture_stdout/capture_stderr should be capture_output/capture_error_output.

Works for me.

-- 
Jan Cholasta