[issue32424] Synchronize copy methods between Python and C implementations of xml.etree.ElementTree.Element

[Stefan Behnel]

Change by Stefan Behnel :


--
resolution:  -> fixed
stage: patch review -> resolved
status: open -> closed

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue32424] Synchronize copy methods between Python and C implementations of xml.etree.ElementTree.Element

[Stefan Behnel]

Stefan Behnel  added the comment:


New changeset 7d952ded6813c896ea3f4234bb8db5247dcb5484 by Stefan Behnel (Gordon 
P. Hemsley) in branch 'master':
bpo-32424: Deprecate xml.etree.ElementTree.Element.copy() in favor of 
copy.copy() (GH-12995)
https://github.com/python/cpython/commit/7d952ded6813c896ea3f4234bb8db5247dcb5484


--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue32424] Synchronize copy methods between Python and C implementations of xml.etree.ElementTree.Element

[Stefan Behnel]

Stefan Behnel  added the comment:

Let's just deprecate it directly (in Py3.9, I guess), given that it was never 
documented.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue32424] Synchronize copy methods between Python and C implementations of xml.etree.ElementTree.Element

[Gordon P. Hemsley]

Gordon P. Hemsley  added the comment:

It seems the final open question on this is whether the mismatch between the 
Python and C implementations is enough to bypass PendingDeprecationWarning for 
copy() in favor of jumping straight to DeprecationWarning.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue32424] Synchronize copy methods between Python and C implementations of xml.etree.ElementTree.Element

[Gordon P. Hemsley]

Change by Gordon P. Hemsley :


--
pull_requests: +12918

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue32424] Synchronize copy methods between Python and C implementations of xml.etree.ElementTree.Element

[Stefan Behnel]

Stefan Behnel  added the comment:


New changeset 50fed0b64faa305338ef5607b570fe209de6 by Stefan Behnel (Gordon 
P. Hemsley) in branch 'master':
bpo-32424: Improve test coverage for xml.etree.ElementTree (GH-12891)
https://github.com/python/cpython/commit/50fed0b64faa305338ef5607b570fe209de6


--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue32424] Synchronize copy methods between Python and C implementations of xml.etree.ElementTree.Element

[Gordon P. Hemsley]

Change by Gordon P. Hemsley :


--
pull_requests: +12822

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue32424] Synchronize copy methods between Python and C implementations of xml.etree.ElementTree.Element

[Serhiy Storchaka]

Serhiy Storchaka  added the comment:

Concur with Stefan. Adding the method deprecated from the born looks silly. We 
should either deprecate the copy() method in the Python implementation, or add 
an undeprecated method in the C implementation. The latter should be considered 
as a new feature.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue32424] Synchronize copy methods between Python and C implementations of xml.etree.ElementTree.Element

[Stefan Behnel]

Stefan Behnel  added the comment:

We should not add anything to the implementation that we consider legacy 
elsewhere. Py3 has "always" used the C accelerator module instead of the Python 
implementation, which suggests that its interface is probably the righter one.

So: make sure that the Obvious Ways, copy.copy() and copy.deepcopy(), work 
nicely for both implementations, and deprecate Element.copy() in Py3.8.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue32424] Synchronize copy methods between Python and C implementations of xml.etree.ElementTree.Element

[Gordon P. Hemsley]

Gordon P. Hemsley  added the comment:

Opened issue36685 for discussion of the attrib copy issue.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue32424] Synchronize copy methods between Python and C implementations of xml.etree.ElementTree.Element

[Gordon P. Hemsley]

Gordon P. Hemsley  added the comment:

Taking a step back, I want to make sure I understand the action items for 
resolving this:
* Add copy() as an alias to __copy__() in the C implementation to match the 
Python implementation.
* Deprecate copy() in favor of copy.copy() in both the Python and C 
implementations, bypassing a PendingDeprecation step because the 
implementations have been out of sync for so long as it is.

Any other issues discussed here are orthogonal to the goal of this ticket.

Are we in agreement on this?

--
versions: +Python 3.8, Python 3.9 -Python 3.7

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue32424] Synchronize copy methods between Python and C implementations of xml.etree.ElementTree.Element

[Serhiy Storchaka]

Serhiy Storchaka  added the comment:

> As it stands, calling Element.__deepcopy__() will succeed with the C
> implementation and fail with the Python implementation.

You should not call it directly. Use copy.deepcopy().

> What is
> different about the C implementation that requires the definition of
> __deepcopy__() in the first place?

__deepcopy__() in the C implementation is merely optimization. It is less 
efficient to convert from the efficient internal representation of Element 
object to tuples and dicts and back.

> However, create_new_element()
> does not make a copy of the attrib dict that is passed in, meaning that
> the internal attrib dict of an Element instance is mutable by changing
> the dict that was passed in.

create_new_element() usually is called with a new copy of the attrib dict. 
Making yet one copy inside it is just a waste of time. If in some cases it is 
called with an externally referred dict, fix these cases by making a copy 
before calling create_new_element().

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue32424] Synchronize copy methods between Python and C implementations of xml.etree.ElementTree.Element

[Gordon P. Hemsley]

Gordon P. Hemsley  added the comment:

>> As discussed above, starting with msg309074, __deepcopy__() is being added 
>> to the Python implementation because it already exists in the C 
>> implementation.
> 
> Python implementation shouldn't copy all implementation details of the C 
> implementation. This is cumbersome and often impossible. Both implementation 
> should have the same behavior, and they do have.

As it stands, calling Element.__deepcopy__() will succeed with the C
implementation and fail with the Python implementation. That seems
problematic to me, and making that *not* be the case is trivial, so I
don't understand the generalization to "all implementation details".

Additionally, if copy.deepcopy() already works well with the Python
implementation, why would it not work on the C implementation? What is
different about the C implementation that requires the definition of
__deepcopy__() in the first place?

>> And additional tests have in fact uncovered further discrepancies between 
>> the Python and C implementations with regard to copying behavior.
> 
> What are these discrepancies?

As I mentioned in the PR, the C implementation does not make use of
Element.__init__() when it creates new instances of the class, opting
instead to use create_new_element() (which is not used in the C
implementation of Element.__init__()). However, create_new_element()
does not make a copy of the attrib dict that is passed in, meaning that
the internal attrib dict of an Element instance is mutable by changing
the dict that was passed in.

I have modified the C implementation to fix this problem by adding a
copy line in create_new_element(). Without this change, some of the new
tests in the PR will fail in the C implementation.

The Python implementation does not have this problem because it
inherently always goes through Element.__init__().

> In any case it is better to extend existing tests by adding missed checks 
> than duplicate tests.

As I mentioned in the PR:

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.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue32424] Synchronize copy methods between Python and C implementations of xml.etree.ElementTree.Element

[Serhiy Storchaka]

Serhiy Storchaka  added the comment:

> As discussed above, starting with msg309074, __deepcopy__() is being added to 
> the Python implementation because it already exists in the C implementation.

Python implementation shouldn't copy all implementation details of the C 
implementation. This is cumbersome and often impossible. Both implementation 
should have the same behavior, and they do have.

> And additional tests have in fact uncovered further discrepancies between the 
> Python and C implementations with regard to copying behavior.

What are these discrepancies?

In any case it is better to extend existing tests by adding missed checks than 
duplicate tests.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue32424] Synchronize copy methods between Python and C implementations of xml.etree.ElementTree.Element

[Gordon P. Hemsley]

Gordon P. Hemsley  added the comment:

As discussed above, starting with msg309074, __deepcopy__() is being added to 
the Python implementation because it already exists in the C implementation.

And additional tests have in fact uncovered further discrepancies between the 
Python and C implementations with regard to copying behavior.

PR 5046 has changes that I believe are ready for review.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue32424] Synchronize copy methods between Python and C implementations of xml.etree.ElementTree.Element

[Serhiy Storchaka]

Serhiy Storchaka  added the comment:

There is an existing test for copying. No new tests are needed. Just add a case 
for the copy() method in test_copy.

Special __deepcopy__() method for Python implementation is not needed. 
copy.deepcopy() already works well.

--
nosy: +serhiy.storchaka

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue32424] Synchronize copy methods between Python and C implementations of xml.etree.ElementTree.Element

[Gordon P. Hemsley]

Gordon P. Hemsley  added the comment:

Given the discussion, I've gone ahead and created a new PR that synchronizes 
the three copy methods between implementations and deprecates Element.copy().

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue32424] Synchronize copy methods between Python and C implementations of xml.etree.ElementTree.Element

[Gordon P. Hemsley]

Change by Gordon P. Hemsley :


--
title: Rename copy() to __copy__() in xml.etree.ElementTree.Element Python 
implementation -> Synchronize copy methods between Python and C implementations 
of xml.etree.ElementTree.Element

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com