Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

The new email API should use MappingProxyType instead of returning new dicts. #66190

Closed
bitdancer opened this issue Jul 16, 2014 · 18 comments
Closed

The new email API should use MappingProxyType instead of returning new dicts. #66190

bitdancer opened this issue Jul 16, 2014 · 18 comments
Labels
easy topic-email type-bug An unexpected behavior, bug, or error

Comments

@bitdancer
Copy link
Member

BPO 21991
Nosy @warsaw, @vstinner, @bitdancer, @matrixise
Files
  • issue21991.patch
    		•

    Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.

    Show more details

    GitHub fields:

    assignee = None
closed_at = <Date 2014-10-17.23:34:23.246>
created_at = <Date 2014-07-16.14:29:58.858>
labels = ['easy', 'type-bug', 'expert-email']
title = 'The new email API should use MappingProxyType instead of returning new dicts.'
updated_at = <Date 2014-10-18.08:24:18.292>
user = 'https://github.com/bitdancer'

    bugs.python.org fields:

    activity = <Date 2014-10-18.08:24:18.292>
actor = 'matrixise'
assignee = 'none'
closed = True
closed_date = <Date 2014-10-17.23:34:23.246>
closer = 'r.david.murray'
components = ['email']
creation = <Date 2014-07-16.14:29:58.858>
creator = 'r.david.murray'
dependencies = []
files = ['36935']
hgrepos = []
issue_num = 21991
keywords = ['patch', 'easy']
message_count = 18.0
messages = ['223211', '224797', '229251', '229254', '229299', '229300', '229306', '229321', '229413', '229425', '229426', '229435', '229436', '229442', '229452', '229616', '229617', '229636']
nosy_count = 8.0
nosy_names = ['barry', 'vstinner', 'r.david.murray', 'jesstess', 'python-dev', 'matrixise', 'Lita.Cho', 'saikrishna17394']
pr_nums = []
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'behavior'
url = 'https://bugs.python.org/issue21991'
versions = ['Python 3.5']
    @bitdancer
    Copy link
    Member Author

    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.

    @bitdancer bitdancer added topic-email easy type-bug An unexpected behavior, bug, or error labels Jul 16, 2014
    @LitaCho
    Copy link
    Mannequin

    LitaCho mannequin commented Aug 5, 2014

    Hi David, I can take this on as I am learning the email api currently.

    @matrixise
    Copy link
    Member

    David,

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

    Thanks

    @bitdancer
    Copy link
    Member Author

    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.

    @matrixise
    Copy link
    Member

    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.

    @matrixise
    Copy link
    Member

    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.

    @matrixise
    Copy link
    Member

    Here is the new version of this patch with a test.

    @bitdancer
    Copy link
    Member Author

    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.

    @matrixise
    Copy link
    Member

    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 ;-)

    @matrixise
    Copy link
    Member

    New version of the patch

    @matrixise
    Copy link
    Member

    sorry, but how to update a patch without losing the rietveld review? Is there a documentation about that?

    Thanks.

    Stephane

    @vstinner
    Copy link
    Member

    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.

    @vstinner
    Copy link
    Member

    bpo-21991.patch looks good to me. I didn't check if more methos should be modified. David knows that better than me :-)

    @bitdancer
    Copy link
    Member Author

    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.

    @vstinner
    Copy link
    Member

    Personally I would test that the returned object is read only

    I agree: write a short helper to check that modifying the dict fails.

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Oct 17, 2014

    New changeset fea3ddcaf652 by R David Murray in branch '3.4':
    bpo-21991: make headerregistry params property MappingProxyType.
    https://hg.python.org/cpython/rev/fea3ddcaf652

    New changeset 5beb1ea76f36 by R David Murray in branch 'default':
    Merge: bpo-21991: make headerregistry params property MappingProxyType.
    https://hg.python.org/cpython/rev/5beb1ea76f36

    @bitdancer
    Copy link
    Member Author

    Thanks, Stéphane. I committed the fix with a modified test.

    @matrixise
    Copy link
    Member

    Thanks David,

    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Assignees
    No one assigned
    Labels
    easy topic-email type-bug An unexpected behavior, bug, or error
    Projects
    None yet
    Milestone
    No milestone
    Development

    No branches or pull requests
    3 participants
    @matrixise @vstinner @bitdancer