bpo-32424: Synchronize xml.etree.ElementTree.Element copy methods. #5046

GPHemsley · 2017-12-29T16:25:31Z

The Python implementation had Element.copy() while the C
implementation had Element.__copy__() and Element.__deepcopy__().

This synchronizes the two implementations and deprecates
elem.copy() in favor of copy.copy(elem).

https://bugs.python.org/issue32424

pganssle · 2017-12-29T17:43:33Z

Lib/test/test_xml_etree.py

+        self.assertEqual(element_foo2.tail, self.element_foo.tail)
+
+        for (child1, child2) in zip(self.element_foo, element_foo2):
+            self.assertIs(child1, child2)


I dunno if this is a problem with the code coverage software or what, but it doesn't seem like this statement is getting hit by the coverage checker. Is it possible that one of these two has length zero?

I'll note that I personally prefer to use itertools.zip_longest for tests like this, since zip will just silently fail if one of the two iterators is a truncated version of the other - e.g. all(x == y for x, y in zip((1, 2, 3), (1, 2, 3, 4, 5))).

The reason the statement wasn't getting hit was because I forgot to actually check it.

And while the lists are expected to be of identical length, I agree that failing silently is not a good idea.

I've created a new commit that addresses these two issues, plus others.

serhiy-storchaka

__deepcopy__ is not needed in Python implementation. Many tests duplicate already existing tests or expose implementation details.

serhiy-storchaka · 2017-12-31T06:27:29Z

Lib/test/test_xml_etree.py

@@ -102,6 +103,11 @@ def newtest(*args, **kwargs):
    return decorator


+#-----------------------------------------------------------------------------


This comment is redundant.

serhiy-storchaka · 2017-12-31T06:28:01Z

Lib/test/test_xml_etree.py

@@ -115,6 +121,143 @@ def test_all(self):
        support.check__all__(self, ET, names, blacklist=("HTML_EMPTY",))


+class ElementTree_Element_UnitTest(unittest.TestCase):


The name of this class is not in line with other names.

serhiy-storchaka · 2017-12-31T06:29:49Z

Lib/test/test_xml_etree.py

+    def test___init__(self):
+        tag = "foo"
+        attrib = { "zix": "wyp" }
+


Too much empty lines. Remove unneeded empty lines.

serhiy-storchaka · 2017-12-31T06:30:33Z

Lib/test/test_xml_etree.py

+
+        element_foo = ET.Element(tag, attrib)
+
+        with self.subTest("traits of an element"):


This is not needed. It doesn't match the style of other tests.

serhiy-storchaka · 2017-12-31T06:36:06Z

Lib/test/test_xml_etree.py

+
+        with self.subTest("traits of an element"):
+            self.assertIsInstance(element_foo, ET.Element)
+            self.assertIn("tag", dir(element_foo))


This is already tested in test_interface.

serhiy-storchaka · 2017-12-31T06:43:36Z

Lib/test/test_xml_etree.py

+            attrib["bar"] = "baz"
+
+            self.assertIsNot(element_foo.attrib, attrib)
+            self.assertNotEqual(element_foo.attrib, attrib)


This is already tested in test_attrib.

serhiy-storchaka · 2017-12-31T06:46:10Z

Lib/test/test_xml_etree.py

+            self.assertIsNot(element_foo2.attrib, element_foo.attrib)
+            self.assertNotEqual(element_foo2.attrib, element_foo.attrib)
+
+    def test___copy__(self):


There is an existing test for copying: test_copy.

serhiy-storchaka · 2017-12-31T06:49:24Z

Lib/test/test_xml_etree.py

-        del e1, e2, e3
-        gc_collect()
-        self.assertIsNone(wref())
+        with self.subTest("Test the shortest cycle: d->element->d"):


Unrelated changes.

These changes helped me to debug the changes I made to the C implementation.

Then please remove them after finishing debugging.

My point was that they were not wholly unrelated. What is the harm in including them here?

I agree with @GPHemsley that it's better to have subtests than multiple tests in a single test function. Although having multiple test functions also wouldn't be all too bad. :)
Yes, they are unrelated to this specific ticket, but they don't really seem big enough to merit their own ticket and PR. They somewhat match the general refactoring of lengthy tests into subtests that this change does in other places. It's at least an edge case.

serhiy-storchaka · 2017-12-31T06:50:24Z

Lib/xml/etree/ElementTree.py

        elem = self.makeelement(self.tag, self.attrib)
        elem.text = self.text
        elem.tail = self.tail
        elem[:] = self
        return elem

+    def __deepcopy__(self, memo):


This method is not needed in Python implementation.

I would prefer you have this discussion on the bpo ticket, as PEP 399 suggests otherwise.

This doesn't contradict PEP 399.

The intention of PEP-399 is that both implementations behave the same. It's not that they use exactly the same mechanisms to reach that goal. Some things work in Python that don't easily work in C, and vice-versa. That's why there are two implementations in the first place.

If we can avoid adding Python (or C) code without sacrificing that goal, then that is a good thing. Code that does not exist is entirely free of bugs, for example. :)

serhiy-storchaka · 2017-12-31T06:51:37Z

Modules/_elementtree.c

@@ -283,10 +283,12 @@ create_new_element(PyObject* tag, PyObject* attrib)
    PyObject_GC_Track(self);

    if (attrib != Py_None && !is_empty_dict(attrib)) {
+        attrib = PyDict_Copy(attrib);


Why this change has been added?

Because the results of the tests in this commit uncovered a discrepancy between the C and Python implementations whereby the attrib list is not copied in the C version beyond the __init__ function. (The Python implementation always uses __init__ whenever it creates a new element, but the C implementation uses create_new_element in all locations except the original __init__ function.)

In many case this makes a redundant copying since create_new_element is called after copying a dict.

Can this be moved out of this function into the caller(s) that need it?

bedevere-bot · 2017-12-31T06:53:22Z

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.

GPHemsley · 2018-01-15T02:04:36Z

Generally: I have added the unit tests in the way that I have because the existing checks are generally integration tests, and most of the test coverage for this module comes as a side effect of pickle tests. It is my goal to expand the unit tests later to test each method individually, so starting that work here makes it easier.

scoder · 2019-04-14T08:56:07Z

Modules/_elementtree.c

+_elementtree_Element_copy_impl(ElementObject *self)
+/*[clinic end generated code: output=84660e8524276b22 input=1f8134305a7719a3]*/
+{
+    if (PyErr_WarnEx(PyExc_DeprecationWarning,


I'm personally ok with deprecating this right away (lxml never provided such a .copy() method, AFAICT), but still, the usual process would be to issue a PendingDeprecationWarning first.

scoder

I agree with @serhiy-storchaka that some cleanup is needed to get this merged.

The Python implementation had Element.copy() while the C implementation had Element.__copy__() and Element.__deepcopy__(). This synchronizes the two implementations and deprecates elem.copy() in favor of copy.copy(elem).

GPHemsley · 2019-04-14T22:51:16Z

@scoder My latest comment on bpo-32424 explains some of my decisions. Would you mind providing feedback on that?

(In the meantime, I've rebased this against master.)

GPHemsley · 2019-04-20T23:03:54Z

Upon further review, it's clear I'm trying to do too many (4) different things in this one commit, so I'm going to break it up and submit the changes separately.

the-knights-who-say-ni added the CLA signed label Dec 29, 2017

bedevere-bot added the awaiting review label Dec 29, 2017

pganssle reviewed Dec 29, 2017

View reviewed changes

GPHemsley force-pushed the bpo-32424-2 branch from dd7421c to d072962 Compare December 31, 2017 04:10

serhiy-storchaka requested changes Dec 31, 2017

View reviewed changes

bedevere-bot added awaiting changes and removed awaiting review labels Dec 31, 2017

scoder reviewed Apr 14, 2019

View reviewed changes

scoder requested changes Apr 14, 2019

View reviewed changes

GPHemsley force-pushed the bpo-32424-2 branch from d072962 to 822358d Compare April 14, 2019 22:47

GPHemsley mentioned this pull request Apr 15, 2019

Bot breaks existing links that contain bpo numbers python/bedevere#164

Closed

GPHemsley closed this Apr 20, 2019

GPHemsley mentioned this pull request Apr 20, 2019

bpo-32424: Improve test coverage for xml.etree.ElementTree #12891

Merged

GPHemsley mannequin mentioned this pull request Jun 28, 2022

Synchronize copy methods between Python and C implementations of xml.etree.ElementTree.Element #76605

Closed

		@@ -102,6 +103,11 @@ def newtest(args, *kwargs):
		return decorator


		#-----------------------------------------------------------------------------

		@@ -115,6 +121,143 @@ def test_all(self):
		support.check__all__(self, ET, names, blacklist=("HTML_EMPTY",))


		class ElementTree_Element_UnitTest(unittest.TestCase):


		element_foo = ET.Element(tag, attrib)

		with self.subTest("traits of an element"):

Uh oh!

bpo-32424: Synchronize xml.etree.ElementTree.Element copy methods. #5046

bpo-32424: Synchronize xml.etree.ElementTree.Element copy methods. #5046

Uh oh!

Conversation

GPHemsley commented Dec 29, 2017 • edited by bedevere-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

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!

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-bot commented Dec 31, 2017

Uh oh!

GPHemsley commented Jan 15, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

scoder left a comment

Choose a reason for hiding this comment

Uh oh!

GPHemsley commented Apr 14, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

GPHemsley commented Apr 20, 2019

Uh oh!

Uh oh!

GPHemsley commented Dec 29, 2017 •

edited by bedevere-bot

Loading

GPHemsley commented Apr 14, 2019 •

edited

Loading