Skip to content

provide tools and prompts to clone child element if the child already has a parent #186

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 8 commits into
base: main
Choose a base branch
from
Open

provide tools and prompts to clone child element if the child already has a parent #186

Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
821bee4
code fix
rl-utility-man Jan 9, 2025
fa67593
minor cleanup
rl-utility-man Jan 21, 2025
7b5b1bf
[pre-commit.ci] auto fixes from pre-commit.com hooks
pre-commit-ci[bot] Jan 21, 2025
7d677c3
Merge branch 'parent-overwrite-on-reuse-fix' of https://github.com/rl…
rl-utility-man Jan 21, 2025
1a94689
[pre-commit.ci] auto fixes from pre-commit.com hooks
pre-commit-ci[bot] Jan 21, 2025
ee407cc
changed from cloning to issuing a warning
rl-utility-man Feb 4, 2025
a257c83
resolved merge conflict
rl-utility-man Feb 4, 2025
47edfb6
[pre-commit.ci] auto fixes from pre-commit.com hooks
pre-commit-ci[bot] Feb 4, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 28 additions & 1 deletion branca/element.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
import warnings
from binascii import hexlify
from collections import OrderedDict
from copy import copy
from html import escape
from os import urandom
from pathlib import Path
Expand Down Expand Up @@ -88,6 +89,14 @@ def __setstate__(self, state: dict):

self.__dict__.update(state)

def clone(self):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add a type hint:

Suggested change
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)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

clone._id = hexlify(urandom(16)).decode()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

clone._parent = None
return clone

def get_name(self) -> str:
"""Returns a string representation of the object.
This string has to be unique and to be a python and
Expand Down Expand Up @@ -142,7 +151,25 @@ def add_child(
name: Optional[str] = None,
index: Optional[int] = None,
) -> "Element":
"""Add a child."""
"""Add a child and modify the child with a pointer to its parent."""

if child._parent is not None:
warnings.warn(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd also try to make the message shorter.

f"""The code added\n {child.get_name()} as a child of
{self.get_name()} which
overwrote an existing pointer to a parent object, {child._parent.get_name()},
but leaves the parent object pointing to the child.
Branca is designed so each object will have no more than
one parent. Consider replacing the object with object.clone()
so that each object has only one parent. Otherwise, actions may
fail. For example, adding an icon object to a map
only adds one valid icon to the map regardless of how many times
the code adds that icon object. This behavior is documented at:
https://github.com/python-visualization/folium/issues/1885
""",
category=UserWarning,
stacklevel=2,
)
if name is None:
name = child.get_name()
if index is None:
Expand Down