bpo-36144: Add union operators to WeakValueDictionary584#19127

curtisbucher · 2020-03-23T22:15:36Z

@brandtbucher

https://bugs.python.org/issue36144

Updated weakref.py Added tests Updated docs Updated news

brandtbucher

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

Looks great, a couple of comments though:

brandtbucher · 2020-03-23T22:20:27Z

Lib/test/test_weakref.py

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


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}

brandtbucher · 2020-03-23T22:24:34Z

Lib/test/test_weakref.py

+        wvd2 = weakref.WeakValueDictionary({1 : b, 2 : a})
+        wvd3 = wvd1.copy()
+        d1 = {1 : c, 3 : b}
+        pairs = [(5, c), (5, b)]


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

brandtbucher · 2020-03-23T22:26:26Z

Lib/test/test_weakref.py

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


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)

brandtbucher · 2020-03-23T22:28:55Z

Doc/library/weakref.rst

+   .. versionchanged:: 3.9
+      Added support for ``|`` and ``|=`` operators, specified in :pep:`584`.


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

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

Yep, nice catch.

Fixed styling and readibility.

brandtbucher

Looks good!

aeros

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.

Doc/library/weakref.rst

Grammar Co-Authored-By: Kyle Stanley <aeros167@gmail.com>

gvanrossum

Yup.

bedevere-bot · 2020-03-25T01:51:32Z

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

gvanrossum · 2020-03-25T01:52:34Z

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

bedevere-bot · 2020-03-25T02:39:46Z

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

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

What do you need to do:

Don't panic.
Check the buildbot page in the devguide if you don't know what the buildbots are or how they work.
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.
Check if the failure is related to this commit (8f1ed21) or if it is a false positive.
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 · 2020-03-25T13:44:31Z

Buildbot failure looks unrelated.

gvanrossum · 2020-03-25T14:47:15Z

Agreed

curtisbucher added 2 commits March 18, 2020 14:53

Added | and |= operators to WeakValueDictionary

bc4e7e7

Updated weakref.py Added tests Updated docs Updated news

Update test_weakref.py and weakref.py

cef9fd2

the-knights-who-say-ni added the CLA signed label Mar 23, 2020

bedevere-bot added the awaiting review label Mar 23, 2020

brandtbucher requested changes Mar 23, 2020

View reviewed changes

bedevere-bot added awaiting core review and removed awaiting review labels Mar 23, 2020

brandtbucher added the type-feature A feature request or enhancement label Mar 23, 2020

curtisbucher added 2 commits March 23, 2020 15:38

Update weakref.rst and test_weakref.py

3829134

Fixed styling and readibility.

Update weakref.rst

f1b6195

brandtbucher approved these changes Mar 23, 2020

View reviewed changes

brandtbucher requested a review from gvanrossum March 23, 2020 23:20

aeros reviewed Mar 24, 2020

View reviewed changes

Doc/library/weakref.rst Outdated Show resolved Hide resolved

Update Doc/library/weakref.rst

ea5465a

Grammar Co-Authored-By: Kyle Stanley <aeros167@gmail.com>

gvanrossum approved these changes Mar 25, 2020

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting core review labels Mar 25, 2020

gvanrossum merged commit 8f1ed21 into python:master Mar 25, 2020

bedevere-bot removed the awaiting merge label Mar 25, 2020

curtisbucher deleted the WeakValueDictionary584 branch March 25, 2020 20:29

		.. versionchanged:: 3.9
		Added support for ``\|`` and ``\|=`` operators, specified in :pep:`584`.

Uh oh!

Conversation

curtisbucher commented Mar 23, 2020 • edited by bedevere-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

brandtbucher left a comment

Choose a reason for hiding this comment

Uh oh!

brandtbucher Mar 23, 2020

Choose a reason for hiding this comment

Uh oh!

brandtbucher Mar 23, 2020

Choose a reason for hiding this comment

Uh oh!

brandtbucher Mar 23, 2020

Choose a reason for hiding this comment

Uh oh!

brandtbucher Mar 23, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

curtisbucher Mar 23, 2020

Choose a reason for hiding this comment

Uh oh!

brandtbucher Mar 23, 2020

Choose a reason for hiding this comment

Uh oh!

brandtbucher left a comment

Choose a reason for hiding this comment

Uh oh!

aeros left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

gvanrossum left a comment

Choose a reason for hiding this comment

Uh oh!

bedevere-bot commented Mar 25, 2020

Uh oh!

gvanrossum commented Mar 25, 2020

Uh oh!

bedevere-bot commented Mar 25, 2020

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

Uh oh!

brandtbucher commented Mar 25, 2020

Uh oh!

gvanrossum commented Mar 25, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

curtisbucher commented Mar 23, 2020 •

edited by bedevere-bot

Loading

brandtbucher Mar 23, 2020 •

edited

Loading

aeros left a comment •

edited

Loading