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