Author gphemsley
Recipients asvetlov, eli.bendersky, gphemsley, mdk, p-ganssle, r.david.murray, scoder, serhiy.storchaka, thatiparthy
Date 2018-01-20.22:20:28
SpamBayes Score -1.0
Marked as misclassified Yes
Message-id <a1aebec2-7601-5112-8ab2-5fcfed30ed2c@gphemsley.org>
In-reply-to <1516003950.96.0.467229070634.issue32424@psf.upfronthosting.co.za>
Content
>> 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.
History
Date User Action Args
2018-01-20 22:20:29gphemsleysetrecipients: + gphemsley, scoder, r.david.murray, eli.bendersky, asvetlov, serhiy.storchaka, thatiparthy, mdk, p-ganssle
2018-01-20 22:20:29gphemsleylinkissue32424 messages
2018-01-20 22:20:28gphemsleycreate