Skip to content

Conversation

@m-janicki
Copy link

@m-janicki m-janicki commented May 17, 2025

Fix #291

I'm not entirely convinced this solution is valid. Maybe the merge_update function should accept a frozen_merge keyword argument. This argument would be set to True in the __add__ method, allowing all the new code to be conditional within an if statement?

Comment on lines +842 to +845
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
Copy link
Owner

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

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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

2 participants