provide tools and prompts to clone child element if the child already has a parent #186
Conversation
for more information, see https://pre-commit.ci
…-utility-man/branca into parent-overwrite-on-reuse-fix
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
rl-utility-man
changed the title
There was a problem hiding this comment.
Hi @rl-utility-man, thanks for taking an interest and trying to improve this aspect of the library. I added some comments. I'm mostly interested in how robust this copy method is, I hope we can discuss that. Thanks!
| """Add a child and modify the child with a pointer to its parent.""" | ||
|
|
||
| if child._parent is not None: | ||
| warnings.warn( |
There was a problem hiding this comment.
I'm a bit conflicted about this warning. On the one hand, it's good to warn for potential issues. On the other hand, there are cases where you want to overwrite the _parent attribute, and generating warnings will lead to confusion. An example is the current workaround we have for using one Icon with multiple Markers: python-visualization/folium#2068
Maybe as a compromise, instead of using the warnings library, instead use a logging message.
logger = logging.getLogger("branca")
logger.warn("bla"
What do you think?
There was a problem hiding this comment.
I'd also try to make the message shorter.
| @@ -88,6 +89,14 @@ def __setstate__(self, state: dict): | |||
|
|
|||
| self.__dict__.update(state) | |||
|
|
|||
| def clone(self): | |||
There was a problem hiding this comment.
Add a type hint:
| def clone(self): | |
| def clone(self) -> 'Element': |
| """creates a new copy of an element, but with a unique identifier | ||
| and without the prior version's relationship to a parent object""" | ||
| clone = copy(self) | ||
| clone._id = hexlify(urandom(16)).decode() |
There was a problem hiding this comment.
we should split this logic off into a _create_id method or something, since we use it now in two places. But okay if you don't do that in this PR.
| def clone(self): | ||
| """creates a new copy of an element, but with a unique identifier | ||
| and without the prior version's relationship to a parent object""" | ||
| clone = copy(self) |
There was a problem hiding this comment.
I wonder about cases where this copy is not sufficient.
- What if the element has children?
- Does it matter if we don't create a new Template object?
- Folium has all kinds of classes, maybe there are some that have class attributes for which a simple copy is not sufficient?
I'm not sure how we can know this clone method works robustly for all Folium classes.
And if we use deepcopy, is that too much?
I reported Folium issue 1885, which occurs because passing a single element to the add_child routine twice overwrites the first value of the parent variable inside the element object. @Conengmo suggested adding a clone routine to address this problem, which I have done. Applying add_child(exampleChild) always overwrites the value of the exampleChild.parent variable, which will lead to undesirable behavior, so I changed add_child to emit a warning that recommends submitting a clone instead when users try to do that.
Thanks for your help. Looking forward to discussing the best way forward.