Skip to content

Conversation

@Wulian233
Copy link
Contributor

@Wulian233Wulian233 commented Jul 10, 2025

@Wulian233
Copy link
ContributorAuthor

Wulian233 commented Jul 14, 2025

Supplement: #136507 (comment) , so this only in 3.14 and 3.15.

CC: @hugovk could you review this?

@mpkocher
Copy link
Contributor

It's probably a good idea to add both success and failure test case for the multiple provided values (e.g., -e image/jpeg text/html, and -e image/jpeg text/xyz).

@encukou
Copy link
Member

Could you also test the failure case?

@hugovkhugovk changed the title gh-136507: Fix mimetypes CLI cannot handle multiple file parametersgh-136507: Fix mimetypes CLI to handle multiple file parametersJul 22, 2025
@Wulian233
Copy link
ContributorAuthor

Could you also test the failure case?

Sorry, I might not have understood it well. 9500cb3 Run this test was failing before the fix, now it runs pass after the fix

…nEuGS.rst Co-authored-by: Hugo van Kemenade <1324225+hugovk@users.noreply.github.com>
else:
sys.exit(f"error: media type unknown for {gtype}")
return'\n'.join(results)
returnhelp_text
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is return help_text line used?

Either:

  • delete return help_text (that branch of the code is never used)
  • change return help_text to return "\n".join(results) and delete the other return calls at the end of each for type in args.type block.

Copy link
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not used, IDE already given a hint

In addition, test failure case have been added

@mpkocher
Copy link
Contributor

Could you also test the failure case?

Sorry, I might not have understood it well. 9500cb3 Run this test was failing before the fix, now it runs pass after the fix

I believe there needs to be a test for the sys.exit calls. For example the case of error: media type unknown for dragons to stderr with a non-zero exit code.

Note that this appears to be slightly changing the interface from a streaming model to a fail fast on the first error case.

@Wulian233Wulian233 requested a review from hugovkAugust 11, 2025 02:02
@hugovk
Copy link
Member

With this PR we get both results when successful:

./python.exe -m mimetypes 1.pdf 1.pngtype: application/pdf encoding: Nonetype: image/png encoding: None

Or if it fails on the last one:

./python.exe -m mimetypes 1.pdf 1.pnxtype: application/pdf encoding: Noneerror: media type unknown for 1.pnx

But not if it fails on the first one (or both):

./python.exe -m mimetypes 1.pdx 1.pnxerror: media type unknown for 1.pdx

Shall we collect all results and display them all at the end?

./python.exe -m mimetypes 1.pdx 1.pngerror: media type unknown for 1.pdxtype: image/png encoding: None

Something like this:

diff --git a/Lib/mimetypes.py b/Lib/mimetypes.py index d38c6e1aba2..7d0f4c1fd40 100644 --- a/Lib/mimetypes.py+++ b/Lib/mimetypes.py@@ -718,8 +718,6 @@ def _parse_args(args): def _main(args=None): """Run the mimetypes command-line interface and return a text to print.""" - import sys- args, help_text = _parse_args(args) results = [] @@ -729,17 +727,21 @@ def _main(args=None): if guess: results.append(str(guess)) else: - sys.exit(f"error: unknown type{gtype}")- return '\n'.join(results)+ results.append(f"error: unknown type{gtype}")+ return results else: for gtype in args.type: guess, encoding = guess_type(gtype, not args.lenient) if guess: results.append(f"type:{guess} encoding:{encoding}") else: - sys.exit(f"error: media type unknown for{gtype}")- return '\n'.join(results)+ results.append(f"error: media type unknown for{gtype}")+ return results if __name__ == '__main__': - print(_main())+ import sys++ results = _main()+ print("\n".join(results))+ sys.exit(any(result.startswith("error: ") for result in results))

@Wulian233
Copy link
ContributorAuthor

Very thanks! Applyed and fixed test

Copy link
Member

@hugovkhugovk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!

@encukouencukou merged commit 81268a3 into python:mainAug 25, 2025
48 checks passed
@encukouencukou added the needs backport to 3.14 bugs and security fixes label Aug 25, 2025
@miss-islington-app
Copy link

Thanks @Wulian233 for the PR, and @encukou for merging it 🌮🎉.. I'm working now to backport this PR to: 3.14.
🐍🍒⛏🤖

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Aug 25, 2025
…pythonGH-136508) (cherry picked from commit 81268a3) Co-authored-by: Wulian233 <1055917385@qq.com> Co-authored-by: Hugo van Kemenade <1324225+hugovk@users.noreply.github.com>
@bedevere-app
Copy link

GH-138140 is a backport of this pull request to the 3.14 branch.

@bedevere-appbedevere-appbot removed the needs backport to 3.14 bugs and security fixes label Aug 25, 2025
@Wulian233Wulian233 deleted the mime branch August 25, 2025 21:57
@mpkocher
Copy link
Contributor

if name == 'main':

  • print(_main())
  • import sys
  • results = _main()
  • print("\n".join(results))
  • sys.exit(any(result.startswith("error: ") for result in results))

Why not encapsulate the functionality in _main and have it return tuple[string, int] with the second parameter communicating the exit code?

Also, it would be better if errors were written to stderr, not stdout.

encukou added a commit that referenced this pull request Oct 7, 2025
GH-136508) (GH-138140) (cherry picked from commit 81268a3) Co-authored-by: Wulian233 <1055917385@qq.com> Co-authored-by: Hugo van Kemenade <1324225+hugovk@users.noreply.github.com> Co-authored-by: Petr Viktorin <encukou@gmail.com>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@Wulian233@mpkocher@encukou@hugovk