Skip to content

fix: adding frozen boxes #292

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 2 commits into
base: master
Choose a base branch
from
Open

fix: adding frozen boxes #292

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
edfbbc4
fix: adding frozen boxes
m-janicki May 17, 2025
4f3a979
test: adding frozen boxes result in frozen box
m-janicki May 30, 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
3 changes: 3 additions & 0 deletions box/box.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -839,7 +839,10 @@ def convert_and_set(k, v):
# in the `converted` box_config set
v = self._box_config["box_class"](v, **self.__box_config(extra_namespace=k))
if k in self and isinstance(self[k], dict):
frozen = self[k]._box_config['frozen_box']
self[k]._box_config['frozen_box'] = False
self[k].merge_update(v, box_merge_lists=merge_type)
self[k]._box_config['frozen_box'] = frozen
Comment on lines +842 to +845
Copy link
Owner

@cdgriffith cdgriffith May 30, 2025

Choose a reason for hiding this comment

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

Suggested change
frozen = self[k]._box_config['frozen_box']
self[k]._box_config['frozen_box'] = False
self[k].merge_update(v, box_merge_lists=merge_type)
self[k]._box_config['frozen_box'] = frozen
self[k]._box_config["frozen_box"] = self._box_config["frozen_box"]
self[k].merge_update(v, box_merge_lists=merge_type)

I think it might make more sense to just set it to the parent's setting, so it should always be as expected.

Copy link
Author

@m-janicki m-janicki May 30, 2025

Choose a reason for hiding this comment

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

That makes sense, but I'm afraid it breaks frozen attribute for nested data. I've added 2 more test cases, ran this code against them, but one of them failed

Box/test/test_box.py

Lines 999 to 1004 in 4f3a979

def test_adding_nested_frozen_boxes_result_in_frozen_box(self):
a = Box({'one': {"two": '1.2'} }, frozen_box=True)
b = Box({'one': {"three": '1.3'} }, frozen_box=True)
c = a + b
with pytest.raises(BoxError):
c.one.four = '1.4'

I think here is what happens:

  1. In __add__ method frozen is temporarily set to False (see code)
  2. Later, when running self[k]._box_config["frozen_box"] = self._box_config["frozen_box"] The frozen = False is assigned to the nested box, and it is never set to True.

return
if isinstance(v, list) and not intact_type:
v = box.BoxList(v, **self.__box_config(extra_namespace=k))
Expand Down
5 changes: 5 additions & 0 deletions test/test_box.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -984,6 +984,11 @@ def test_add_boxes(self):
with pytest.raises(BoxError):
Box() + BoxList()

def test_add_frozen_boxes(self):
b = Box(c=1, d={"sub": 1}, e=1, frozen_box=True)
c = dict(d={"val": 2}, e=4)
assert b + c == Box(c=1, d={"sub": 1, "val": 2}, e=4)

def test_iadd_boxes(self):
b = Box(c=1, d={"sub": 1}, e=1)
c = dict(d={"val": 2}, e=4)
Expand Down