bpo-36144: Implement defaultdict union. #18729
Conversation
Lib/test/test_defaultdict.py
Outdated
| s = defaultdict(str, {0: "zero", 1: "one"}) | ||
|
|
||
| self.assertIs((i | s).default_factory, int) | ||
| self.assertDictEqual(i | s, {1: "one", 2: 2, 0: "zero"}) |
There was a problem hiding this comment.
Is defaultdict not order-preserving, like dict? If it is, could the test check it?
There was a problem hiding this comment.
You found a flaw in my merge logic! I pushed a fix and added tests for key order.
| @@ -183,5 +183,30 @@ def test_pickling(self): | |||
| o = pickle.loads(s) | |||
| self.assertEqual(d, o) | |||
|
|
|||
| def test_union(self): | |||
There was a problem hiding this comment.
defaultdict inherits a perfectly fine |= from dict.
I can add tests for it here if you'd like, but the tests for dict already have the same coverage.
There was a problem hiding this comment.
D'oh. Maybe add a comment to this effect?
Codecov Report
@@ Coverage Diff @@
## master #18729 +/- ##
===========================================
+ Coverage 82.06% 83.22% +1.15%
===========================================
Files 1956 1571 -385
Lines 589514 415419 -174095
Branches 44464 44488 +24
===========================================
- Hits 483809 345733 -138076
+ Misses 96052 60034 -36018
+ Partials 9653 9652 -1
Continue to review full report at Codecov.
|
Lib/test/test_defaultdict.py
Outdated
| self.assertDictEqual(i | s, {1: "one", 2: 2, 0: "zero"}) | ||
| self.assertEqual(list(i | s), [1, 2, 0]) | ||
| self.assertIs((s | i).default_factory, str) |
There was a problem hiding this comment.
Maybe introduce an intermediate variable, e.g. i_s = i | s to make it abundantly clear that these three assertions are about the same result? Etc.
Modules/_collectionsmodule.c
Outdated
| @@ -1990,6 +1990,13 @@ defdict_missing(defdictobject *dd, PyObject *key) | |||
| return value; | |||
| } | |||
|
|
|||
| static inline PyObject* | |||
| defdict_copy_with_map(defdictobject *dd, PyObject *map) | |||
There was a problem hiding this comment.
map is a slightly confusing or over-specified name for the argument -- it really represents anything you can pass to dict(), so it could be a mapping (we don't call those maps except in some internals I believe) or an iterable of (key, value) pairs. Maybe just arg?
Also the name of the function is getting awkward. I wonder if it should just be called "new_defdict"? (Arguably the caller could pass in the default factory and the logic here could just replace NULL with Py_None?)
Modules/_collectionsmodule.c
Outdated
| static PyObject* | ||
| defdict_or(PyObject* left, PyObject* right) | ||
| { | ||
| int lhs = PyObject_IsInstance(left, (PyObject*)&defdict_type); |
There was a problem hiding this comment.
Sorry about harping on names! To me, lhs is a noun, but this variable is a condition -- the meaning of the condition seems to be the negation of "should the arguments be swapped". If it weren't for the negation I'd name it "swap" -- but given that, maybe "left_is_self"?
|
I agree, thanks! |
|
Oh, an incredibly minor piece of feedback: in the Subject/Title/Whatever line of commits, issues, PRs and the like, we prefer no final period. These things are like headlines in a newspaper. I don't know it's in the dev guide, but I'm sure I've seen this advice elsewhere, and it's a common convention. |
https://bugs.python.org/issue36144