classification
Title: The new email API should use MappingProxyType instead of returning new dicts.
Type: behavior Stage: resolved
Components: email Versions: Python 3.5
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: Lita.Cho, barry, jesstess, matrixise, python-dev, r.david.murray, saikrishna17394, vstinner
Priority: normal Keywords: easy, patch

Created on 2014-07-16 14:29 by r.david.murray, last changed 2014-10-18 08:24 by matrixise. This issue is now closed.

Files
File name Uploaded Description Edit
issue21991.patch matrixise, 2014-10-15 09:54 review
Messages (18)
msg223211 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2014-07-16 14:29
There are a few places in the new email API where dicts are returned containing what is conceptually static information.  Currently this is done by returning a copy of the dict from the object, so that user code modifying the dict won't break the object invariants.  It would be better to change these to MappingProxyType objects instead, before the API moves out of provisional status.

This issue is mostly a note to myself, since I'm the most likely to be able to figure out which places in the code need changing, but if anyone else wants to look at it feel free, since it will probably be a while before I get to it.
msg224797 - (view) Author: Lita Cho (Lita.Cho) * Date: 2014-08-05 03:07
Hi David, I can take this on as I am learning the email api currently.
msg229251 - (view) Author: Stéphane Wirtel (matrixise) * Date: 2014-10-13 16:37
David,

do you have an example, I am at the CPython sprint in Dublin, and I think I can work on this issue.

Thanks
msg229254 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2014-10-13 17:04
The principle example is the 'params' dictionary in headerregistry.  Currently it gets recreated every time you access that attribute.  You can *apparently* change it, but that has no real effect.  Probably the computed value should be cached the first time the attribute is accessed, and a MappingProxy over the cached value returned.
msg229299 - (view) Author: Stéphane Wirtel (matrixise) * Date: 2014-10-14 15:10
Hi David,

I didn't find an other example of a copy(dict), the rest is just some lists. If you have an other example in the email library, I will agree to provide an other patch.
msg229300 - (view) Author: Stéphane Wirtel (matrixise) * Date: 2014-10-14 15:12
In fact, I am really dubious with my patch because this one is really small and I think there is a missing part somewhere because the description of this issue takes 4 lines and the patch only 2.
msg229306 - (view) Author: Stéphane Wirtel (matrixise) * Date: 2014-10-14 15:49
Here is the new version of this patch with a test.
msg229321 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2014-10-14 17:31
No, it looks fine.  This issue was mostly a reminder to myself.  Thanks for the patch.

The other place I thought there might be some instances of this was in _header_value_parser, but I don't see any on a quick scan.  So this may be it.
msg229413 - (view) Author: Stéphane Wirtel (matrixise) * Date: 2014-10-15 08:34
Tell me if you will review this patch and maybe accept it ;-)

In fact I checked all the return keywords in the email library and I didn't find any other copy of a dict.

This is the reason why I am dubious about my patch ;-)
msg229425 - (view) Author: Stéphane Wirtel (matrixise) * Date: 2014-10-15 09:54
New version of the patch
msg229426 - (view) Author: Stéphane Wirtel (matrixise) * Date: 2014-10-15 09:54
sorry, but how to update a patch without losing the rietveld review? Is there a documentation about that?

Thanks.

Stephane
msg229435 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2014-10-15 11:50
> sorry, but how to update a patch without losing the rietveld review?

I see 3 patch sets at:
http://bugs.python.org/review/21991/

No review is lost when you remove a patch. But it's better to attach a new patch with a different name. I like the name pattern: name.patch, name-2.patch, name-3.patch, etc. It's easier to refer to a patch when it has a unique name.
msg229436 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2014-10-15 11:51
issue21991.patch looks good to me. I didn't check if more methos should be modified. David knows that better than me :-)
msg229442 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2014-10-15 13:10
Personally I would test that the returned object is read only, rather than checking for MappingProxyType explicitly, but you can argue either way as being better :)

As for other occurrences, I must have been either misremembering, or I refactored the other occurrences out of existence earlier.

Yes I will apply this.
msg229452 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2014-10-15 14:26
> Personally I would test that the returned object is read only

I agree: write a short helper to check that modifying the dict fails.
msg229616 - (view) Author: Roundup Robot (python-dev) Date: 2014-10-17 23:33
New changeset fea3ddcaf652 by R David Murray in branch '3.4':
#21991: make headerregistry params property MappingProxyType.
https://hg.python.org/cpython/rev/fea3ddcaf652

New changeset 5beb1ea76f36 by R David Murray in branch 'default':
Merge: #21991: make headerregistry params property MappingProxyType.
https://hg.python.org/cpython/rev/5beb1ea76f36
msg229617 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2014-10-17 23:34
Thanks, Stéphane.  I committed the fix with a modified test.
msg229636 - (view) Author: Stéphane Wirtel (matrixise) * Date: 2014-10-18 08:24
Thanks David,
History
Date User Action Args
2014-10-18 08:24:18matrixisesetmessages: + msg229636
2014-10-17 23:34:23r.david.murraysetstatus: open -> closed
resolution: fixed
messages: + msg229617

stage: commit review -> resolved
2014-10-17 23:33:02python-devsetnosy: + python-dev
messages: + msg229616
2014-10-15 14:26:33vstinnersetmessages: + msg229452
2014-10-15 13:10:24r.david.murraysetmessages: + msg229442
stage: patch review -> commit review
2014-10-15 11:51:21vstinnersetmessages: + msg229436
2014-10-15 11:50:37vstinnersetnosy: + vstinner
messages: + msg229435
2014-10-15 09:54:44matrixisesetmessages: + msg229426
2014-10-15 09:54:08matrixisesetfiles: + issue21991.patch

messages: + msg229425
2014-10-15 09:53:31matrixisesetfiles: - issue21991.patch
2014-10-15 09:28:22berker.peksagsetstage: needs patch -> patch review
2014-10-15 08:34:01matrixisesetmessages: + msg229413
2014-10-14 17:31:24r.david.murraysetmessages: + msg229321
2014-10-14 15:49:48matrixisesetfiles: + issue21991.patch

messages: + msg229306
2014-10-14 15:49:13matrixisesetfiles: - issue21991.patch
2014-10-14 15:12:34matrixisesetmessages: + msg229300
2014-10-14 15:10:40matrixisesetfiles: + issue21991.patch
keywords: + patch
messages: + msg229299
2014-10-13 17:04:04r.david.murraysetmessages: + msg229254
2014-10-13 16:37:30matrixisesetnosy: + matrixise
messages: + msg229251
2014-09-26 00:07:20saikrishna17394setnosy: + saikrishna17394
2014-08-05 03:42:17Lita.Chosetnosy: + jesstess
2014-08-05 03:07:26Lita.Chosetnosy: + Lita.Cho
messages: + msg224797
2014-07-16 14:29:58r.david.murraycreate