gh-131178: Fix mimetypes CLI to write error messages to stderr#149683
gh-131178: Fix mimetypes CLI to write error messages to stderr#149683htjworld wants to merge 5 commits into
Conversation
Documentation build overview
9 files changed ·
|
htjworld
changed the title
htjworld
changed the title
htjworld
changed the title
sobolevn
left a comment
sobolevn
left a comment
There was a problem hiding this comment.
This is a rather hard problem: when docs do not represent the reality, we can either fix docs or fix the code.
I guess a lot of places already rely on the fact that errors and results are in the stdout, not in stderr.
So, I think that it would be better to change the docs.
|
|
||
| def test_type_lookup(self): | ||
| rc, stdout, stderr = assert_python_ok('-m', 'mimetypes', 'foo.pdf') | ||
| self.assertIn(b'application/pdf', stdout) |
There was a problem hiding this comment.
Why not asserting the full stdout contents?
| def test_type_lookup_unknown(self): | ||
| rc, stdout, stderr = assert_python_failure('-m', 'mimetypes', 'foo.unknownext12345') | ||
| self.assertEqual(stdout, b'') | ||
| self.assertIn(b'error:', stderr) |
There was a problem hiding this comment.
And the same here: we can assert the full error response.
| has_error = False | ||
| for result in results: | ||
| if result.startswith("error: "): | ||
| print(result, file=sys.stderr, flush=True) |
There was a problem hiding this comment.
This is a behavior change, please revert it.
| @@ -0,0 +1,2 @@ | |||
| Fix :mod:`mimetypes` CLI to write error messages to stderr instead of stdout, | |||
There was a problem hiding this comment.
And since we revert the behavior change, we need to remove this file as well :)
Per sobolevn's review: revert the behavior change that routed error messages to stderr, as existing code may rely on stdout behavior. Instead update docs to accurately reflect that errors go to stdout. Also update tests to assert full output contents as suggested.
|
I have made the requested changes; please review again |
|
Thanks for making the requested changes! : please review the changes made to this pull request. |
|
|
||
| def test_unknown_flag(self): | ||
| rc, stdout, stderr = assert_python_failure('-m', 'mimetypes', '--unknown-flag') | ||
| self.assertNotEqual(rc, 0) |
| @@ -0,0 +1,2 @@ | |||
| Add subprocess-based tests for the :mod:`mimetypes` command-line interface, | |||
There was a problem hiding this comment.
we don't add news for docs / tests only changes.
|
|
||
| def test_extension_flag(self): | ||
| rc, stdout, stderr = assert_python_ok('-m', 'mimetypes', '-e', 'image/jpeg') | ||
| self.assertIn(b'.jpg', stdout) |
There was a problem hiding this comment.
Let's use assertEqual here as well.
|
thanks for the review appreciate it |
|
Thanks for making the requested changes! : please review the changes made to this pull request. |
2 participants
The
mimetypesdocumentation states that errors go to the standard error stream, but the CLI has always written all output — including errors — to stdout.Fix the documentation to match the actual behavior, and add subprocess-based tests for the command-line interface.
Changes:
Doc/library/mimetypes.rst: Correct "standard error stream" → "standard output stream"; update stale examples (filename.pictis now a recognized type, error message strings did not match actual output).Lib/test/test_mimetypes.py: Add subprocess-based tests verifying CLI output format and exit codes.