Skip to content

Commit

Permalink
clean up input check in dict_deep_merge
Browse files Browse the repository at this point in the history
  • Loading branch information
bgunnar5 committed May 9, 2024
1 parent 023737f commit daf22ec
Show file tree
Hide file tree
Showing 2 changed files with 11 additions and 43 deletions.
44 changes: 7 additions & 37 deletions merlin/utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -557,28 +557,6 @@ def needs_merlin_expansion(
return False


def _both_inputs_are_dicts(dict_a: dict, dict_b: dict):
"""
Helper function for `dict_deep_merge` to ensure both entries are dictionaries.
:param `dict_a`: A dict that we'll merge dict_b into
:param `dict_b`: A dict that we want to merge into dict_a
"""
dict_a_is_dict = isinstance(dict_a, dict)
dict_b_is_dict = isinstance(dict_b, dict)
if not dict_a_is_dict or not dict_b_is_dict:
if not dict_a_is_dict and not dict_b_is_dict:
problem_str = f"both dict_a '{dict_a}' and dict_b '{dict_b}' are not dictionaries"
elif not dict_a_is_dict:
problem_str = f"dict_a '{dict_a}' is not a dictionary"
elif not dict_b_is_dict:
problem_str = f"dict_b '{dict_b}' is not a dictionary"

LOG.warning(f"Problem with dict_deep_merge: {problem_str}. Ignoring this merge call.")
return False
return True


def dict_deep_merge(dict_a: dict, dict_b: dict, path: str = None, conflict_handler: Callable = None):
"""
This function recursively merges dict_b into dict_a. The built-in
Expand All @@ -596,23 +574,15 @@ def dict_deep_merge(dict_a: dict, dict_b: dict, path: str = None, conflict_handl
"""

# Check to make sure we have valid dict_a and dict_b input
if not _both_inputs_are_dicts(dict_a, dict_b):
msgs = [
f"{name} '{actual_dict}' is not a dict"
for name, actual_dict in [("dict_a", dict_a), ("dict_b", dict_b)]
if not isinstance(actual_dict, dict)
]
if len(msgs) > 0:
LOG.warning(f"Problem with dict_deep_merge: {', '.join(msgs)}. Ignoring this merge call.")
return

# # Check to make sure we have valid dict_a and dict_b input
# dict_a_is_dict = isinstance(dict_a, dict)
# dict_b_is_dict = isinstance(dict_b, dict)
# if not dict_a_is_dict or not dict_b_is_dict:
# if not dict_a_is_dict and not dict_b_is_dict:
# problem_str = f"both dict_a '{dict_a}' and dict_b '{dict_b}' are not dictionaries"
# elif not dict_a_is_dict:
# problem_str = f"dict_a '{dict_a}' is not a dictionary"
# elif not dict_b_is_dict:
# problem_str = f"dict_b '{dict_b}' is not a dictionary"

# LOG.warning(f"Problem with dict_deep_merge: {problem_str}. Ignoring this merge call.")
# return

if path is None:
path = []
for key in dict_b:
Expand Down
10 changes: 4 additions & 6 deletions tests/unit/utils/test_dict_deep_merge.py
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ def run_invalid_check(dict_a: Any, dict_b: Any, expected_log: str, caplog: "Fixt
assert dict_a_initial == dict_a

# Check that dict_deep_merge logs a warning
print(f"caplog.text: {caplog.text}")
assert expected_log in caplog.text, "Missing expected log message"


Expand Down Expand Up @@ -69,10 +70,7 @@ def test_dict_deep_merge_both_dicts_invalid(dict_a: Any, dict_b: Any, caplog: "F
"""

# The expected log that's output by dict_deep_merge
expected_log = (
f"Problem with dict_deep_merge: both dict_a '{dict_a}' "
f"and dict_b '{dict_b}' are not dictionaries. Ignoring this merge call."
)
expected_log = f"Problem with dict_deep_merge: dict_a '{dict_a}' is not a dict, dict_b '{dict_b}' is not a dict. Ignoring this merge call."

# Run the actual test
run_invalid_check(dict_a, dict_b, expected_log, caplog)
Expand Down Expand Up @@ -101,7 +99,7 @@ def test_dict_deep_merge_dict_a_invalid(dict_a: Any, dict_b: Dict[str, str], cap
"""

# The expected log that's output by dict_deep_merge
expected_log = f"Problem with dict_deep_merge: dict_a '{dict_a}' is not a dictionary. Ignoring this merge call."
expected_log = f"Problem with dict_deep_merge: dict_a '{dict_a}' is not a dict. Ignoring this merge call."

# Run the actual test
run_invalid_check(dict_a, dict_b, expected_log, caplog)
Expand Down Expand Up @@ -130,7 +128,7 @@ def test_dict_deep_merge_dict_b_invalid(dict_a: Dict[str, str], dict_b: Any, cap
"""

# The expected log that's output by dict_deep_merge
expected_log = f"Problem with dict_deep_merge: dict_b '{dict_b}' is not a dictionary. Ignoring this merge call."
expected_log = f"Problem with dict_deep_merge: dict_b '{dict_b}' is not a dict. Ignoring this merge call."

# Run the actual test
run_invalid_check(dict_a, dict_b, expected_log, caplog)
Expand Down

0 comments on commit daf22ec

Please sign in to comment.