bpo-34938: mimetypes.init() does not overwrite globals#16567
bpo-34938: mimetypes.init() does not overwrite globals#16567AWhetter wants to merge 6 commits intopython:mainfrom
Conversation
|
Hello, and thanks for your contribution! I'm a bot set up to make sure that the project can legally accept this contribution by verifying everyone involved has signed the PSF contributor agreement (CLA). CLA MissingOur records indicate the following people have not signed the CLA: For legal reasons we need all the people listed to sign the CLA before we can look at your contribution. Please follow the steps outlined in the CPython devguide to rectify this issue. If you have recently signed the CLA, please wait at least one business day You can check yourself to see if the CLA has been received. Thanks again for the contribution, we look forward to reviewing it! |
38dc050 to
8105b34
Compare
|
Thanks for your time @AWhetter, and welcome to CPython! 😎 I assume you've seen the bot's message about the CLA? |
There was a problem hiding this comment.
This looks like a good fix. I think it also needs a regression test in Lib/test/test_mimetypes.py.
Looking at the failed test, though, it seems that the current behavior of creating new objects rather than updating the existing ones is expected (though I think I agree with you that it's flawed).
Maybe someone from @python/email-team can shed some light on the desired behavior here?
|
I have seen the CLA notice. I've already signed up so I'm waiting for the approval to go through the system at the moment. Thanks for the fast feedback! I'll get a test added! |
When importing mimetypes.init() into a module scope with any of encodings_map, suffix_map, types_map, or common_types, subsequent calls to init() would update the maps in the local module scope and not in the mimetypes module. This change makes the original maps always get updated.
8105b34 to
ffcb6c1
Compare
|
I posted on the bpo issue, as my comments ended up at a higher level than simple PR feedback. Also, no need to force push changes. We'll squash merge at the end, and force pushing only breaks the review system. |
|
@maxking is this PR ready to merge? @zooba can you check the question in https://bugs.python.org/issue34938#msg359070, do you expect those to be worked on this PR, or separate? Thanks. |
|
I've made the suggested documentation changes. The bit about importing |
YoSTEALTH
left a comment
There was a problem hiding this comment.
There is no need to have line: 388-392 and line: 571 in Lib/mimetypes.py is there? Like take out the _default_mime_types function and indent back the content. Thanks
Lib/mimetypes.py
Outdated
| @@ -388,21 +391,23 @@ def _default_mime_types(): | |||
| global types_map, _types_map_default | |||
| global common_types, _common_types_default | |||
There was a problem hiding this comment.
These global statements and the function (and its call) can be removed.
YoSTEALTH
left a comment
There was a problem hiding this comment.
these change must be reverted back, you need to have it as:
suffix_map = _suffix_map_default.copy() not suffix_map = _suffix_map_default = {...} because they are suppose to be 2 separate variable. On
Line 366 in d8fb9e1
suffix_map.clear() is called and if they are same variable it will be bad.
iritkatriel
left a comment
There was a problem hiding this comment.
This PR has merge conflicts now.
|
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 |
When importing mimetypes.init() into a module scope with
any of encodings_map, suffix_map, types_map, or common_types,
subsequent calls to init() would update the maps in
the local module scope and not in the mimetypes module.
This change makes the original maps always get updated.
This will break the undocumented behaviour of any changes to
mimetypes.encodings_map,mimetypes.suffix_map,mimetypes.types_map, ormimetypes.common_typespersisting after callingmimetypes.init()because the maps and their equivalentmimetypes._default<map>variable are now two separate dictionaries.https://bugs.python.org/issue34938