borb icon indicating copy to clipboard operation
borb copied to clipboard

BUG: Deepcopy always return a Dictionary rather than the copied object class

Open kriansa opened this issue 1 year ago • 2 comments

Describe the bug deepcopy on a Dictionary descendant object will always make them another Dictionary rather than the callee object class.

To Reproduce Steps to reproduce the behaviour:

from borb.pdf.page.page import Page

page = Page()
print(type(page)) # <class 'borb.pdf.page.page.Page'>

newpage = copy.deepcopy(page)
print(type(newpage)) # <class 'borb.io.read.types.Dictionary'>

Expected behaviour Copying an object should return a new one with the same object type.

Additional context This is probably the culprit, but I may lack the skill to tell what's the right decision here.

kriansa avatar Aug 23 '22 01:08 kriansa

That's not the only place though. Few descendant classes overrides __init__ arity (Page, PageInfo and Annotation), so it won't be as easy as to change that call to type(self)(). Maybe those should also override __deepcopy__?

It's worth mentioning that borb.io.read.types.Function and borb.pdf.canvas.font.font.Font overrides __deepcopy__, so those are probably fine as they are (but tests should enforce that).

kriansa avatar Aug 23 '22 03:08 kriansa

Replacing that method by

def __deepcopy__(self, memodict={}):
    out = type(self).__new__(type(self))
    Dictionary.__init__(out)
                                                                     
    for k, v in self.items():
        out[copy.deepcopy(k, memodict)] = copy.deepcopy(v, memodict)
    return out

has worked out for me, but I haven't tested it thoroughly. LMK what are your thoughts on the idea.

kriansa avatar Aug 23 '22 03:08 kriansa

Hi there,

Sorry for the belated reply. It seems like your suggestion fixes the deepcopy issue. I added your code, and added more tests to check whether a copied object matches the return type of its original. You may expect to find this code in the next release.

Kind regards, Joris Schellekens

jorisschellekens avatar Sep 26 '22 10:09 jorisschellekens

@jorisschellekens oh, no problem! Thank you for merging it!

kriansa avatar Sep 28 '22 06:09 kriansa