gh-145056: Let dict specific tests accept frozendict as well by rhettinger · Pull Request #145060 · python/cpython
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not ready yet.
| d |= list(b.items()) | ||
| expected = OrderedDict({0: 0, 1: 1, 2: 2, 3: 3}) | ||
| self.assertEqual(a | dict(b), expected) | ||
| self.assertEqual(a | frozendict(b), expected) |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should not the type of the result to be tested too?
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The existing test didn't check result type. I don't want to feature creep this edit. Reviewing and expanding the existing test strategies can be a task for another day.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The new tests are passed with unmodified code. Therefore they are not correct tests for this change.
|
|
||
| def __or__(self, other): | ||
| if not isinstance(other, dict): | ||
| if not isinstance(other, (dict, frozendict)): |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about the C implementation?
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The C version already supported any mapping input, that is how the tests passed. But I will update the fast path to use PyAnyDict_CheckExact(arg).
| if isinstance(other, UserDict): | ||
| return self.__class__(self.data | other.data) | ||
| if isinstance(other, dict): | ||
| if isinstance(other, (dict, frozendict)): |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any tests for this?
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, I didn't see where the dict version was tested so left this alone.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very bad. We need to add tests with dict first.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, let's leave this. There are many other missing tests for UserDict and UserList. I am working on this.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#145145 will add new tests. Now there will be a place for new frozendict tests.