Skip to content

bpo-33463: _asdict --> dict, not OrderedDict#6772

Closed
selik wants to merge 13 commits into
python:masterfrom
selik:fix-issue-33463
Closed

bpo-33463: _asdict --> dict, not OrderedDict#6772
selik wants to merge 13 commits into
python:masterfrom
selik:fix-issue-33463

Conversation

@selik

@selik selik commented May 12, 2018

Copy link
Copy Markdown
Contributor

Now that dict is tracking insertion order, namedtuple._asdict can be
switched to returning a basic dict instead of an OrderedDict. The
interface is largely similar, but some minor differences will require a
deprecation cycle before making the change.

https://bugs.python.org/issue33463

Now that dict is tracking insertion order, namedtuple._asdict can be
switched to returning a basic dict instead of an OrderedDict. The
interface is largely similar, but some minor differences will require a
deprecation cycle before making the change.
As per @serhiy-storchaka's suggestion, a warning here is too noisy. The
difference between OrderedDict and dict is now negligible.
@selik selik force-pushed the fix-issue-33463 branch from df3d43e to af20cb9 Compare May 23, 2018 21:00
@selik selik changed the title bpo-33463: warn _asdict --> dict, not OrderedDict bpo-33463: _asdict --> dict, not OrderedDict May 23, 2018
@selik

selik commented Jun 29, 2018

Copy link
Copy Markdown
Contributor Author

I changed the docs to note the change as of Version 3.8. If this is accepted, I'll create separate pull requests to add warnings to the docs of older versions.

@serhiy-storchaka serhiy-storchaka left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Needed a test.

Please add a What's New entry and remove a duplicated news entry, and clarify the reason of reverting the 3.1 change.

Comment thread Doc/library/collections.rst Outdated
Comment thread Doc/library/collections.rst Outdated
@bedevere-bot

Copy link
Copy Markdown

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

@selik

selik commented Sep 13, 2018

Copy link
Copy Markdown
Contributor Author

@serhiy-storchaka Were you saying this needs a test for the returned type? There's already a test verifying that _asdict returns an appropriate dictionary.

self.assertEqual(p._asdict(), dict(x=11, y=22)) # test _asdict method

@serhiy-storchaka

Copy link
Copy Markdown
Member

For testing just add something like:

self.assertEqual(list(p._asdict().items()), [('x': 11), ('y': 22)])

dict equality doesn't take the order into account.

Since this change breaks compatibility, it needs to be mentioned in the What's New document, in the "Porting to 3.8" section.

@selik

selik commented Sep 19, 2018

Copy link
Copy Markdown
Contributor Author

I added a bit to What's New, but I just realized I need to move that to the "Porting to 3.8" section. I'll do that shortly.

@selik

selik commented Sep 19, 2018

Copy link
Copy Markdown
Contributor Author

Ok, I added a short comment about constructing an OrderedDict if using the move_to_end method.

@rhettinger rhettinger self-assigned this Jan 31, 2019
@rhettinger

Copy link
Copy Markdown
Contributor

Thanks for the PR.

Have applied an alternative version so the issue is taken care of.

@rhettinger rhettinger closed this Jan 31, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants