bpo-40497: Fix handling of check in subprocess.check_output() #19897
Conversation
|
I'm only wondering if we should have a deprecation period for disallowing |
| @@ -162,6 +162,14 @@ def test_check_output(self): | |||
| [sys.executable, "-c", "print('BDFL')"]) | |||
| self.assertIn(b'BDFL', output) | |||
|
|
|||
| with self.assertRaisesRegex(ValueError, | |||
| "stdout argument not allowed, it will be overridden"): | |||
There was a problem hiding this comment.
when asserting raised exceptions, don't assert anything about the error message text unless that is critically important. here i would change these to either plain assertRaises(ValueError) or to a very permissive assertRaisesRegex(ValueError, ...) just looking for the word "stdout" in this one and "check" in the one below. The actual text of the error message does not matter and should be able to be tweaked without someone needing to update a change-detector test.
|
regarding deprecation, no need. the argument, if specified at all, is already a TypeError. This change just makes it be the correct ValueError with a friendlier error message. |
| @@ -0,0 +1,3 @@ | |||
| Disallow passing a *check* parameter to :meth:`subprocess.check_output`, | |||
There was a problem hiding this comment.
I think this news entry makes it sound like behavior was changed, and will prompt users to review their codebases for use of check parameter.
It may be better to say that the exception raised was changed from TypeError to ValueError and that error message was changed to be easier to understand. It may be good to mention that it applies to stdout arg as well.
There was a problem hiding this comment.
Agreed, Andrei.
…GH-19897) Co-authored-by: Tal Einat <taleinat@gmail.com> Co-authored-by: Łukasz Langa <lukasz@langa.pl>
https://bugs.python.org/issue40497
The text was updated successfully, but these errors were encountered: