bpo-36144: Implement defaultdict union.#18729
Conversation
gvanrossum
left a comment
gvanrossum
left a comment
There was a problem hiding this comment.
I don’t recall how ‘|=‘ is implemented — does it have some default behavior if ‘|’ is defined?
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.
| 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.
|
gvanrossum
left a comment
gvanrossum
left a comment
There was a problem hiding this comment.
Sorry for being so nitpicky. Also, I didn't even realize I'd found a logic error in your code. Shows you what I know. :-(
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
| } | ||
|
|
||
| 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"?
gvanrossum
left a comment
gvanrossum
left a comment
There was a problem hiding this comment.
Thanks, much more readable!
|
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. |
4 participants
https://bugs.python.org/issue36144