8000 bpo-36144: Add union operators to WeakValueDictionary584 by curtisbucher · Pull Request #19127 · python/cpython · GitHub
[go: up one dir, main page]
Skip to content

bpo-36144: Add union operators to WeakValueDictionary584 #19127

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 5 commits into from
Mar 25, 2020
Merged

bpo-36144: Add union operators to WeakValueDictionary584 #19127

merged 5 commits into from
Mar 25, 2020

Conversation

curtisbucher
Copy link
Contributor
@curtisbucher curtisbucher commented Mar 23, 2020 •
edited by bedevere-bot
Loading

brandtbucher
brandtbucher requested changes Mar 23, 2020
View reviewed changes
Copy link
Member
@brandtbucher brandtbucher left a comment

Choose a reason for hiding this comment

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

Wow, you might get 3 PRs merged in one day!

Looks great, a couple of comments though:

Lib/test/test_weakref.py Outdated
Comment on lines 1616 to 1619
wvd1 = weakref.WeakValueDictionary({1 : a})
wvd2 = weakref.WeakValueDictionary({1 : b, 2 : a})
wvd3 = wvd1.copy()
d1 = {1 : c, 3 : b}
Copy link
Member

Choose a reason for hiding this comment

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

No space before :, ever!

Suggested change
wvd1 = weakref.WeakValueDictionary({1 : a})
wvd2 = weakref.WeakValueDictionary({1 : b, 2 : a})
wvd3 = wvd1.copy()
d1 = {1 : c, 3 : b}
wvd1 = weakref.WeakValueDictionary({1: a})
wvd2 = weakref.WeakValueDictionary({1: b, 2: a})
wvd3 = wvd1.copy()
d1 = {1: c, 3: b}

Lib/test/test_weakref.py Outdated
wvd2 = weakref.WeakValueDictionary({1 : b, 2 : a})
wvd3 = wvd1.copy()
d1 = {1 : c, 3 : b}
pairs = [(5, c), (5, b)]
Copy link
Member

Choose a reason for hiding this comment

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

This is an interesting test case, but I just want to verify that you intended to use the same key twice here.

You didn't mean something like this, which results in a mapping of length 2?

Suggested change
pairs = [(5, c), (5, b)]
pairs = [(5, c), (6, b)]

Lib/test/test_weakref.py Outdated
Comment on lines 1644 to 1647
self.assertNotIn(2, tmp1.keys())
self.assertNotIn(2, tmp2.keys())
self.assertNotIn(1, tmp3.keys())
self.assertNotIn(1, tmp4.keys())
Copy link
Member

Choose a reason for hiding this comment

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

I don't think you need the keys() calls here:

Suggested change
self.assertNotIn(2, tmp1.keys())
self.assertNotIn(2, tmp2.keys())
self.assertNotIn(1, tmp3.keys())
self.assertNotIn(1, tmp4.keys())
self.assertNotIn(2, tmp1)
self.assertNotIn(2, tmp2)
self.assertNotIn(1, tmp3)
self.assertNotIn(1, tmp4)

Doc/library/weakref.rst Outdated
Comment on lines 204 to 205
.. versionchanged:: 3.9
Added support for ``|`` and ``|=`` operators, specified in :pep:`584`.
Copy link
Member
@brandtbucher brandtbucher Mar 23, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I think this is better moved up to line 174, just after the ..note:: (and at the same indentation level).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it should be more like line 200, because I believe line 174 is under WeakKeyDictionary rather than WeakValueDictionary?

Copy link
Member

Choose a reason for hiding this comment

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

Yep, nice catch.

@brandtbucher brandtbucher added the type-feature A feature request or enhancement label Mar 23, 2020
brandtbucher
brandtbucher approved these changes Mar 23, 2020
View reviewed changes
Copy link
Member
@brandtbucher brandtbucher left a comment

Choose a reason for hiding this comment

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

Looks good!

@brandtbucher brandtbucher requested a review from gvanrossum March 23, 2020 23:20
aeros
aeros reviewed Mar 24, 2020
View reviewed changes
Copy link
Contributor
@aeros aeros left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Thanks for the PR @curtisbucher. I agree with the inclusion of a versionadded for support of the union operators on dictionaries, it's a substantial new feature.

aeros
aeros reviewed Mar 24, 2020
View reviewed changes
@curtisbucher @aeros
Update Doc/library/weakref.rst
ea5465a 
Grammar

Co-Authored-By: Kyle Stanley <aeros167@gmail.com>
gvanrossum
gvanrossum approved these changes Mar 25, 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.

@gvanrossum gvanrossum merged commit 8f1ed21 into python:master Mar 25, 2020
@bedevere-bot
Copy link

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

@gvanrossum
Copy link
Member

Oops, screwed up the commit title. Oh well, we'll live. Thanks again @curtisbucher and @brandtbucher !

@bedevere-bot
Copy link

⚠️⚠️⚠️ Buildbot failure ⚠️⚠️⚠️

Hi! The buildbot x86 Gentoo Installed with X 3.x has failed when building commit 8f1ed21.

What do you need to do:

  1. Don't panic.
  2. Check the buildbot page in the devguide if you don't know what the buildbots are or how they work.
  3. Go to the page of the buildbot that failed (https://buildbot.python.org/all/#builders/128/builds/520) and take a look at the build logs.
  4. Check if the failure is related to this commit (8f1ed21) or if it is a false positive.
  5. If the failure is related to this commit, please, reflect that on the issue and make a new Pull Request with a fix.

You can take a look at the buildbot page here:

https://buildbot.python.org/all/#builders/128/builds/520

Failed tests:

  • test_smtpnet

Failed subtests:

  • test_connect_using_sslcontext_verified - test.test_smtpnet.SmtpSSLTest

Summary of the results of the build (if available):

== Tests result: FAILURE then FAILURE ==

406 tests OK.

1 test failed:
test_smtpnet

13 tests skipped:
test_asdl_parser test_check_c_globals test_clinic test_devpoll
test_gdb test_ioctl test_kqueue test_msilib test_startfile
test_winconsoleio test_winreg test_winsound test_zipfile64

1 re-run test:
test_smtpnet

Total duration: 39 min 27 sec

Click to see traceback logs 
Traceback (most recent call last):
  File "/buildbot/buildarea/cpython/3.x.ware-gentoo-x86.installed/build/target/lib/python3.9/test/test_smtpnet.py", line 81, in test_connect_using_sslcontext_verified
    server = smtplib.SMTP_SSL(self.testServer, self.remotePort, context=context)
  File "/buildbot/buildarea/cpython/3.x.ware-gentoo-x86.installed/build/target/lib/python3.9/smtplib.py", line 1034, in __init__
    SMTP.__init__(self, host, port, local_hostname, timeout,
  File "/buildbot/buildarea/cpython/3.x.ware-gentoo-x86.installed/build/target/lib/python3.9/smtplib.py", line 253, in __init__
    (code, msg) = self.connect(host, port)
  File "/buildbot/buildarea/cpython/3.x.ware-gentoo-x86.installed/build/target/lib/python3.9/smtplib.py", line 341, in connect
    (code, msg) = self.getreply()
  File "/buildbot/buildarea/cpython/3.x.ware-gentoo-x86.installed/build/target/lib/python3.9/smtplib.py", line 398, in getreply
    raise SMTPServerDisconnected("Connection unexpectedly closed")
smtplib.SMTPServerDisconnected: Connection unexpectedly closed

@brandtbucher
Copy link
Member

Buildbot failure looks unrelated.

@gvanrossum
Copy link
Member

Agreed

@curtisbucher curtisbucher deleted the WeakValueDictionary584 branch March 25, 2020 20:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aeros aeros aeros left review comments

@gvanrossum gvanrossum gvanrossum approved these changes

@brandtbucher brandtbucher brandtbucher 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.
6 participants
@curtisbucher @bedevere-bot @gvanrossum @brandtbucher @aeros @the-knights-who-say-ni
0