From 707be4030e9221e8180de0198e4282b4d0cb4fcc Mon Sep 17 00:00:00 2001 From: Lukas Gold Date: Wed, 2 Sep 2026 11:53:24 +0200 Subject: [PATCH] fix(wtsite): repair the dangling combine_into reference - add _combine_into, the recursive dict merge update_dict always called - make update_dict a staticmethod, matching how set_value calls it - drop print(match.full_path), which raised TypeError on integer keys - cover all three with offline tests --- src/osw/wtsite.py | 49 +++++++++++--- tests/test_wtpage_update_dict.py | 113 +++++++++++++++++++++++++++++++ 2 files changed, 153 insertions(+), 9 deletions(-) create mode 100644 tests/test_wtpage_update_dict.py diff --git a/src/osw/wtsite.py b/src/osw/wtsite.py index 2c49b787..e1300110 100644 --- a/src/osw/wtsite.py +++ b/src/osw/wtsite.py @@ -50,6 +50,31 @@ } +def _combine_into(update: dict, combined: dict) -> None: + """Merges update into combined in place, recursing into nested dicts + + A nested dict is merged key by key, so keys only present in combined + survive. Any other value replaces what is already there. + + Parameters + ---------- + update + The dict to take the new keys and values from. + combined + The dict to merge into. Modified in place. + """ + for key, value in update.items(): + target = combined.get(key) + if isinstance(value, dict): + if not isinstance(target, dict): + # nothing to merge with, so start from an empty dict rather + # than storing a reference to the one in update + target = combined[key] = {} + _combine_into(value, target) + else: + combined[key] = value + + # Classes class WtSite: """A wrapper class of mwclient.Site, mainly to provide multi-slot page handling and @@ -1698,14 +1723,19 @@ def get_value(self, jsonpath): res.append(match.value) return res + @staticmethod @deprecated("No longer supported") - def update_dict(self, combined: dict, update: dict) -> None: - for k, v in update.items(): - if isinstance(v, dict): - # todo: fix reference for combine_into - wt.combine_into(v, combined.setdefault(k, {})) - else: - combined[k] = v + def update_dict(combined: dict, update: dict) -> None: + """Merges update into combined in place, recursing into nested dicts + + Parameters + ---------- + combined + The dict to merge into. Modified in place. + update + The dict to take the new keys and values from. + """ + _combine_into(update, combined) @deprecated("No longer supported for replace=False") def set_value(self, jsonpath_match, value, replace=False): @@ -1735,10 +1765,11 @@ def set_value(self, jsonpath_match, value, replace=False): # else: jsonpath_expr.update(d, value) matches = jsonpath_expr.find(d) for match in matches: - print(match.full_path) + # str(match.full_path) raises TypeError because the keys of d are the + # list indices, so this cannot be printed or logged as it stands # pprint(value) if not replace: - WtPage.update_dict(match.value, value) + _combine_into(value, match.value) value = match.value # pprint(value) match.full_path.update_or_create(d, value) diff --git a/tests/test_wtpage_update_dict.py b/tests/test_wtpage_update_dict.py new file mode 100644 index 00000000..3755dddb --- /dev/null +++ b/tests/test_wtpage_update_dict.py @@ -0,0 +1,113 @@ +"""Unit tests for WtPage.update_dict and its merge helper. + +Regression guard for #15: update_dict called wt.combine_into, a function that +has never existed in this repository, so every nested dict raised +AttributeError. It was also declared as an instance method but called as +WtPage.update_dict(a, b) in set_value, which raised TypeError before the +missing reference was ever reached. +""" + +from typing import Any, Dict, Union + +from osw.wtsite import WtPage, _combine_into + + +class OfflineWtPage(WtPage): + """A WtPage that never touches a wiki, copied from tests/test_osl.py.""" + + def __init__(self, wtSite: Any = None, title: str = None): + self.wtSite = wtSite + self.title = title + self.exists = True + self._original_content = "" + self.changed: bool = False + self._dict = [] + self._slots: Dict[str, Union[str, dict]] = {"main": ""} + self._slots_changed: Dict[str, bool] = {"main": False} + self._content_model: Dict[str, str] = {"main": "wikitext"} + + +def test_flat_values_are_replaced(): + combined = {"a": 1, "b": 2} + + _combine_into({"b": 3}, combined) + + assert combined == {"a": 1, "b": 3} + + +def test_nested_dicts_are_merged_not_replaced(): + """The point of the recursion: 'keep' has to survive the merge.""" + combined = {"outer": {"keep": 1, "change": 2}} + + _combine_into({"outer": {"change": 3, "add": 4}}, combined) + + assert combined == {"outer": {"keep": 1, "change": 3, "add": 4}} + + +def test_merging_recurses_to_any_depth(): + combined = {"a": {"b": {"c": {"keep": 1}}}} + + _combine_into({"a": {"b": {"c": {"add": 2}}}}, combined) + + assert combined == {"a": {"b": {"c": {"keep": 1, "add": 2}}}} + + +def test_a_dict_replaces_a_scalar(): + combined = {"a": "scalar"} + + _combine_into({"a": {"b": 1}}, combined) + + assert combined == {"a": {"b": 1}} + + +def test_a_scalar_replaces_a_dict(): + combined = {"a": {"b": 1}} + + _combine_into({"a": "scalar"}, combined) + + assert combined == {"a": "scalar"} + + +def test_a_new_nested_key_is_copied_not_aliased(): + """Otherwise editing the result would reach back into the update dict.""" + update = {"a": {"b": 1}} + combined = {} + + _combine_into(update, combined) + combined["a"]["b"] = 2 + + assert update == {"a": {"b": 1}} + + +def test_keys_absent_from_update_are_untouched(): + combined = {"a": 1} + + _combine_into({}, combined) + + assert combined == {"a": 1} + + +def test_update_dict_merges_in_place_and_returns_none(): + combined = {"outer": {"keep": 1}} + + assert WtPage.update_dict(combined, {"outer": {"add": 2}}) is None + assert combined == {"outer": {"keep": 1, "add": 2}} + + +def test_set_value_merges_into_the_existing_entry(): + """set_value(replace=False) is the only caller, and it was broken.""" + page = OfflineWtPage(title="Test") + page._dict = [{"Template": {"keep": "yes", "change": "old"}}] + + page.set_value("$.*.Template", {"change": "new"}) + + assert page._dict == [{"Template": {"keep": "yes", "change": "new"}}] + + +def test_set_value_with_replace_discards_the_existing_entry(): + page = OfflineWtPage(title="Test") + page._dict = [{"Template": {"keep": "yes", "change": "old"}}] + + page.set_value("$.*.Template", {"change": "new"}, replace=True) + + assert page._dict == [{"Template": {"change": "new"}}]