8000 bpo-36144: Update MappingProxyType with PEP 584's operators by brandtbucher · Pull Request #18814 · python/cpython · GitHub
[go: up one dir, main page]
Skip to content

bpo-36144: Update MappingProxyType with PEP 584's operators #18814

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
Merged
merged 9 commits into from
Mar 7, 2020
Merged

bpo-36144: Update MappingProxyType with PEP 584's operators #18814

merged 9 commits into from
Mar 7, 2020

Conversation

brandtbucher
Copy link
Member
@brandtbucher brandtbucher commented Mar 6, 2020 •
edited by bedevere-bot
Loading

@brandtbucher brandtbucher added the type-feature A feature request or enhancement label Mar 6, 2020
@brandtbucher brandtbucher requested a review from gvanrossum March 6, 2020 19:42
gvanrossum
gvanrossum reviewed Mar 7, 2020
View reviewed changes
Copy link
Member
@gvanrossum gvanrossum left a comment

Choose a reason for hiding this comment

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

Hm, since there's no update() method I think it's better if |= raises an exception as well.

@brandtbucher
Copy link
Member Author
brandtbucher commented Mar 7, 2020 •
edited
Loading

Well, I can understand the lack of an update method... we don't want to update this, since it should be read-only.

My intent here was that the x |= y acts as if __ior__ wasn't defined, falling back to x = x | y (like a frozenset). The only difference is that it accepts an iterable of pairs, too... otherwise, I would just leave __ior__ undefined. It's also nice that both operators are pretty thin wrappers around the underlying mappings' implementations of those same operators.

But it's easy enough to change later, if we choose to raise now. I have a slight preference for the way I've written it up, but I'll let you make the call.

@brandtbucher
Copy link
Member Author

If we do raise here, what should the message be? I'd like to at least suggest trying |:

TypeError: |= is not supported by mappingproxy; assign from | instead

@gvanrossum
Copy link
Member

I really think that it's better to reject |= here -- everywhere else (touched by PEP 584) it mutates the object in place. I am not aware of use cases where this class is used other than protecting a class __dict__, and it just feels wrong to have a mutating operator.

I do realize this is a fine line, after all += for numbers and strings returns a copy while += for lists mutates in place. And frozenset has |= that returns a copy while set has |= that mutates. So if you want to ask advice from python-dev I would not object.

How about this error message:

TypeError: '|=' is not supported by mappingproxy; use '|' instead

@brandtbucher
Copy link
Member Author

So if you want to ask advice from python-dev I would not object.

No, you've made good points. I too suspect that this will hardly ever be used... and if enough people disagree, it's easy to change it later!

@gvanrossum
Copy link
Member
gvanrossum commented Mar 7, 2020 via email •
edited
Loading

gvanrossum
gvanrossum approved these changes Mar 7, 2020
View reviewed changes
Copy link
Member
@gvanrossum gvanrossum left a comment

Choose a reason for hiding this comment

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

Yup, that’s good.

I’m not 100% sure that you need to use assertDictEqual—assertEqual is supposed to call it automatically.

@brandtbucher
Copy link
Member Author

I want to assert that the return type is dict, which assertDictEqual enforces!

@brandtbucher
Copy link
Member Author

(as opposed to a new MappingProxy or something)

@gvanrossum
Copy link
Member
gvanrossum commented Mar 7, 2020 via email •
edited
Loading

@gvanrossum gvanrossum merged commit 4663f66 into python:master Mar 7, 2020
@bedevere-bot
Copy link

@gvanrossum: Please replace # with GH- in the commit message next time. Thanks!

sthagen added a commit to sthagen/python-cpython that referenced this pull request Mar 7, 2020
@sthagen
Merge pull request #311 from python/master
721360b 
bpo-36144: Update MappingProxyType with PEP 584's operators (python#18814)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gvanrossum gvanrossum gvanrossum approved these changes

Assignees
No one assigned
Labels
type-feature A feature request or enhancement
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@brandtbucher @gvanrossum @bedevere-bot @the-knights-who-say-ni
0