Skip to content

Comments

gh-145056: Let dict specific tests accept frozendict as well#145060

Open
rhettinger wants to merge 3 commits intopython:mainfrom
rhettinger:collections_any_dict
Open

gh-145056: Let dict specific tests accept frozendict as well#145060
rhettinger wants to merge 3 commits intopython:mainfrom
rhettinger:collections_any_dict

Conversation

@rhettinger
Copy link
Contributor

@rhettinger rhettinger commented Feb 21, 2026

@skirpichev skirpichev self-requested a review February 21, 2026 00:21
@serhiy-storchaka serhiy-storchaka self-requested a review February 21, 2026 10:55
Copy link
Member

@serhiy-storchaka serhiy-storchaka left a comment

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)
Copy link
Member

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?

Copy link
Contributor Author

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.


def __or__(self, other):
if not isinstance(other, dict):
if not isinstance(other, (dict, frozendict)):
Copy link
Member

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?

Copy link
Contributor Author

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)):
Copy link
Member

Choose a reason for hiding this comment

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

Any tests for this?

Copy link
Contributor Author

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.

@bedevere-app
Copy link

bedevere-app bot commented Feb 21, 2026

When you're done making the requested changes, leave the comment: I have made the requested changes; please review again.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

3.15 new features, bugs and security fixes awaiting changes skip news

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants