Skip to content

bpo-34320: Fix dict(o) didn't copy order of dict subclass #8624

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 5 commits into from
Sep 26, 2018
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
35 changes: 35 additions & 0 deletions Lib/test/test_ordered_dict.py
Original file line number Diff line number Diff line change
Expand Up @@ -650,6 +650,41 @@ def test_free_after_iterating(self):
support.check_free_after_iterating(self, lambda d: iter(d.values()), self.OrderedDict)
support.check_free_after_iterating(self, lambda d: iter(d.items()), self.OrderedDict)

# TODO: This test is relating to PEP 468.
# Move this test to somewhere more appropriate.
def test_kwargs_order(self):
Copy link
Member

Choose a reason for hiding this comment

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

This test is about kwargs ordering rather than OrderedDict, so I'd expect it to be next to other tests for kwargs, not here in test_ordered_dict.py.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, but problem is I can't find other tests for kwargs.

Copy link
Member Author

Choose a reason for hiding this comment

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

I added new test class for this, in test_call.py

# bpo-34320
od = self.OrderedDict([('a', 1), ('b', 2)])
od.move_to_end('a')
expected = list(od.items())

def fn(**kw):
return kw

res = fn(**od)
self.assertIsInstance(res, dict)
self.assertEqual(list(res.items()), expected)

# TODO: This test is relating to PEP 520.
Copy link
Member

Choose a reason for hiding this comment

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

This test is not specific to PEP 520, which only relates directly to the class definition namespace. So you should drop this comment.

# Move this test to somewhere more appropriate.
def test_type_namespace_order(self):
Copy link
Member

Choose a reason for hiding this comment

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

This test should be next to other tests of the type() builtin.

# bpo-34320
od = self.OrderedDict([('a', 1), ('b', 2)])
od.move_to_end('a')
expected = list(od.items())

C = type('C', (), od)
self.assertEqual(list(C.__dict__.items())[:2], [('b', 2), ('a', 1)])

def test_dict_copy_order(self):
Copy link
Member

Choose a reason for hiding this comment

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

This test should be in test_dict.py.

# bpo-34320
od = self.OrderedDict([('a', 1), ('b', 2)])
od.move_to_end('a')
expected = list(od.items())

copy = dict(od)
self.assertEqual(list(copy.items()), expected)


class PurePythonOrderedDictTests(OrderedDictTests, unittest.TestCase):

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Fix ``dict(od)`` didn't copy iteration order of OrderedDict.
4 changes: 3 additions & 1 deletion Objects/dictobject.c
Original file line number Diff line number Diff line change
Expand Up @@ -235,6 +235,8 @@ static Py_ssize_t lookdict_split(PyDictObject *mp, PyObject *key,

static int dictresize(PyDictObject *mp, Py_ssize_t minused);

static PyObject* dict_iter(PyDictObject *dict);

/*Global counter used to set ma_version_tag field of dictionary.
* It is incremented each time that a dictionary is created and each
* time that a dictionary is modified. */
Expand Down Expand Up @@ -2379,7 +2381,7 @@ dict_merge(PyObject *a, PyObject *b, int override)
return -1;
}
mp = (PyDictObject*)a;
if (PyDict_Check(b)) {
if (PyDict_Check(b) && (Py_TYPE(b)->tp_iter == (getiterfunc)dict_iter)) {
other = (PyDictObject*)b;
if (other == mp || other->ma_used == 0)
/* a.update(a) or a.update({}); nothing to do */
Expand Down