gh-87901: os.popen: Fix new encoding argument.#92415
gh-87901: os.popen: Fix new encoding argument.#92415methane wants to merge 6 commits intopython:mainfrom
encoding argument.#92415Conversation
encoding keyword onlyencoding argument.
hauntsaninja
left a comment
There was a problem hiding this comment.
Looks good, thanks for fixing!
|
You should also add a unit test that uses this new parameter. |
serhiy-storchaka
left a comment
There was a problem hiding this comment.
Is there a What's New entry?
Lib/test/test_os.py
Outdated
| os.environ.clear() | ||
| os.environ.update(HELLO="World") | ||
| with os.popen("%s -c 'echo $HELLO'" % unix_shell) as popen: | ||
| with os.popen("%s -c 'echo $HELLO'" % unix_shell, encoding="utf-8") as popen: |
There was a problem hiding this comment.
It is not a test for os.popen. It works without explicit encoding, so no need to change it.
Instead look at test_popen. Needed tests for encoding and errors, and they should fail if revert changes in os.popen, so use non-ASCII data and encodings which give different result in comparison with common locale encodings: utf-8, latin1 or cp1252.
There was a problem hiding this comment.
Would it make sense to add a new test for the private attribute to check for the encoding? To check encoding="utf-8".
There was a problem hiding this comment.
I would prefer not to rely on private attributes if possible. Checking the result of the decoding is enough.
Lib/test/test_os.py
Outdated
| os.environ.clear() | ||
| os.environ.update(HELLO="World") | ||
| with os.popen("%s -c 'echo $HELLO'" % unix_shell) as popen: | ||
| with os.popen("%s -c 'echo $HELLO'" % unix_shell, encoding="utf-8") as popen: |
There was a problem hiding this comment.
Would it make sense to add a new test for the private attribute to check for the encoding? To check encoding="utf-8".
|
There is a dedicated test Test:
Include tests for the locale encoding and the filesystem encoding if they are relevant. |
|
os.popen() is just a simple wrapper of subprocess.Popen() |
|
Or should we revert the |
I dislike this API since it always use a shell and so is more likely to be vulnerable to shell code injection. It may also be less efficient than avoiding a shell. I would prefer to deprecate this function, but I was never brave enough to propose deprecating it. At least, I removed platform.popen() ;-) |
|
I created #92836 that remove |
#87901