Skip to content

Conversation

clin1234
Copy link
Contributor

@clin1234 clin1234 commented Oct 20, 2025

Copy link
Member

@AA-Turner AA-Turner left a comment

Choose a reason for hiding this comment

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

Please revert unreleated whitespace changes.

A

@bedevere-app
Copy link

bedevere-app bot commented Oct 21, 2025

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

@clin1234
Copy link
Contributor Author

I have made the requested changes; please review again

@bedevere-app
Copy link

bedevere-app bot commented Oct 21, 2025

Thanks for making the requested changes!

@AA-Turner: please review the changes made to this pull request.

@bedevere-app bedevere-app bot requested a review from AA-Turner October 21, 2025 16:12
Copy link
Member

@StanFromIreland StanFromIreland left a comment

Choose a reason for hiding this comment

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

Please add a NEWS entry.

@@ -0,0 +1,12 @@
Add the following:
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Add the following:
Add the following MIME types:

Comment on lines 2 to 12
- ``application/efi``
- ``application/pem-certificate-chain``
- ``application/pgp-keys``
- ``application/sql``
- ``application/zstd``
- ``application/ocsp-request``
- ``application/ocsp-response``
- ``application/mathematica``
- ``application/mathml+xml``
- ``application/mbox``
- ``application/pem-certificate-chain``
Copy link
Member

Choose a reason for hiding this comment

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

Sort and remove duplicate:

Suggested change
- ``application/efi``
- ``application/pem-certificate-chain``
- ``application/pgp-keys``
- ``application/sql``
- ``application/zstd``
- ``application/ocsp-request``
- ``application/ocsp-response``
- ``application/mathematica``
- ``application/mathml+xml``
- ``application/mbox``
- ``application/pem-certificate-chain``
- ``application/efi``
- ``application/mathematica``
- ``application/mathml+xml``
- ``application/mbox``
- ``application/ocsp-request``
- ``application/ocsp-response``
- ``application/pem-certificate-chain``
- ``application/pgp-keys``
- ``application/sql``
- ``application/zstd``

def test_preferred_extension(self):
def check_extensions():
for mime_type, ext in (
('applicaion/efi', '.efi'),
Copy link
Member

Choose a reason for hiding this comment

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

I know we (and especially me!) have been adding new MIME types to this test, but I'm now thinking there's not much point, we're essentially duplicating types_map in reverse.

It's useful to have some in here, to test the lookup works, but we don't really need all.

When we have multiple extensions (for example, application/mathematica has .ma, .mb and .na), maybe it's useful to include the test to show which extension it returns.

I'm not suggesting removing all existing ones, but shall we remove the new ones here, other than for application/mathematica?

clin1234 and others added 6 commits October 22, 2025 12:12
…sue-140392.ouL6FW.rst

Co-authored-by: Hugo van Kemenade <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants