GeezerVison

The World Wide Web, restored to 800 pixels of glory.

GeezerVison is showing github.com — gh-145056: Let dict specific tests accept frozendict as well by rhettinger · Pull Request #145060 · python/cpython

Conversation

rhettinger commented Feb 21, 2026 •

edited by bedevere-app Bot

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Contributor

Fix the dict specific tests in the collections module.

Issue: Many pure python tools that have a special cases for dict should accept frozendict as well. #145056

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions


          pythongh-145056: Let dict specific tests accept frozendict as well

f27f128

rhettinger assigned skirpichev

Feb 21, 2026

rhettinger added skip news 3.15 bugs and security fixes labels

Feb 21, 2026

bedevere-app Bot added the awaiting core review label

Feb 21, 2026

bedevere-app Bot mentioned this pull request

Feb 21, 2026

Many pure python tools that have a special cases for dict should accept frozendict as well. #145056

Open


          Add tests

4d02992

skirpichev self-requested a review

February 21, 2026 00:21

skirpichev approved these changes

Feb 21, 2026

View reviewed changes

serhiy-storchaka self-requested a review

February 21, 2026 10:55

serhiy-storchaka requested changes

Feb 21, 2026

View reviewed changes

serhiy-storchaka left a comment

Copy link

Copy Markdown

Member

There was a problem hiding this 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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

Lib/test/test_ordered_dict.py

		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)

serhiy-storchaka Feb 21, 2026

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

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?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

rhettinger Feb 21, 2026

Copy link

Copy Markdown

Contributor Author

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

serhiy-storchaka Feb 21, 2026

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

Lib/collections/__init__.py


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

serhiy-storchaka Feb 21, 2026

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

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?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

rhettinger Feb 21, 2026

Copy link

Copy Markdown

Contributor Author

There was a problem hiding this comment.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

Lib/collections/__init__.py

		if isinstance(other, UserDict):
		return self.__class__(self.data \| other.data)
		if isinstance(other, dict):
		if isinstance(other, (dict, frozendict)):

serhiy-storchaka Feb 21, 2026

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Any tests for this?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

rhettinger Feb 21, 2026

Copy link

Copy Markdown

Contributor Author

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

serhiy-storchaka Feb 21, 2026

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

serhiy-storchaka Feb 21, 2026

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

serhiy-storchaka Feb 23, 2026

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

bedevere-app Bot added awaiting changes and removed awaiting core review labels

Feb 21, 2026

bedevere-app Bot commented Feb 21, 2026

Copy link

Copy Markdown

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.


          Expand the fast path in the C version of OrderedDict

a887273

serhiy-storchaka requested changes

Feb 21, 2026

View reviewed changes

Comment thread

Lib/test/test_ordered_dict.py

		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)

serhiy-storchaka Feb 21, 2026

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

Lib/collections/__init__.py

		if isinstance(other, UserDict):
		return self.__class__(self.data \| other.data)
		if isinstance(other, dict):
		if isinstance(other, (dict, frozendict)):

serhiy-storchaka Feb 21, 2026

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

rhettinger closed this

Feb 22, 2026

serhiy-storchaka mentioned this pull request

Mar 22, 2026

gh-145056: Fix merging of OrderedDict and UserDict with frozendict #146295

Closed

skirpichev removed their assignment

May 20, 2026

Reviewers

serhiy-storchaka

serhiy-storchaka requested changes

skirpichev

skirpichev approved these changes

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

3.15

bugs and security fixes

awaiting changes skip news

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

3 participants

Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.

Conversation

rhettinger commented Feb 21, 2026 • edited by bedevere-app Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

serhiy-storchaka left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bedevere-app Bot commented Feb 21, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

rhettinger commented Feb 21, 2026 •

edited by bedevere-app Bot

Loading

Uh oh!

There was an error while loading. Please reload this page.