Skip to content

Commit f41a277

Browse files
fix(compare): spell out which config: values mean no overrides (#5472)
Review follow-up. Only None, "", [] and {} count as an empty config: and are dropped; any other non-mapping (false, 0, a string, a list) is skipped with a warning instead of silently losing the overrides. check_compare_list()'s docstring now states the shape it guarantees, and the comment names where a list config really crashed (overrides looked up by key in run_all()/fetch_config()). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
1 parent 7c50c22 commit f41a277

2 files changed

Lines changed: 19 additions & 11 deletions

File tree

‎apps/predbat/compare.py‎

Lines changed: 9 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -28,7 +28,11 @@
2828

2929

3030
def check_compare_list(compare_list):
31-
"""Split compare_list into the entries that can be compared (ids and names as strings) and a message per skipped entry (#5472)."""
31+
"""Split compare_list into the entries that can be compared and a message per skipped entry (#5472).
32+
33+
Each kept entry is a copy with its id and name as stripped strings, and its config either a
34+
mapping of overrides or absent, so run_all() and fetch_config() can rely on that shape.
35+
"""
3236
if not compare_list:
3337
return [], []
3438
if not isinstance(compare_list, list):
@@ -54,17 +58,17 @@ def check_compare_list(compare_list):
5458
problem = "has a duplicate id '{}'".format(entry_id)
5559
elif not entry_name:
5660
problem = "('{}') requires a non-empty 'name'".format(entry_id)
57-
elif entry.get("config") and not isinstance(entry["config"], dict):
58-
# run_all() snapshots each override by key before any tariff runs, outside every try,
59-
# so a list of overrides there crashed the whole comparison
61+
elif entry.get("config") not in (None, "", []) and not isinstance(entry["config"], dict):
62+
# run_all() and fetch_config() look overrides up by key; a list of them raised TypeError
63+
# there and ended the whole comparison, and any other value was never a usable override
6064
problem = "('{}') has a 'config' that isn't a set of 'setting: value' overrides".format(entry_id)
6165
if problem:
6266
problems.append("compare_list entry {} {}".format(index + 1, problem))
6367
continue
6468
seen_ids.add(entry_id)
6569
checked = dict(entry, id=entry_id, name=entry_name)
6670
if not checked.get("config"):
67-
# An empty 'config:' line (None, [] or "") means no overrides, as it always has
71+
# An empty 'config:' line (None, "", [] or {}) means no overrides, as it always has
6872
checked.pop("config", None)
6973
valid.append(checked)
7074
return valid, problems

‎apps/predbat/tests/test_compare.py‎

Lines changed: 10 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -743,19 +743,23 @@ def _mock_plan_iboost_smart():
743743
{"id": "bad_config", "name": "Bad Config", "config": ["metric_battery_cycle"]},
744744
{"id": "empty_config", "name": "Empty Config", "config": None},
745745
{"id": "empty_list_config", "name": " Empty List Config ", "config": []},
746+
{"id": "empty_string_config", "name": "Empty String Config", "config": ""},
747+
{"id": "empty_dict_config", "name": "Empty Dict Config", "config": {}},
748+
{"id": "false_config", "name": "False Config", "config": False},
749+
{"id": "string_config", "name": "String Config", "config": "best_soc_min: 2.0"},
746750
]
747751
kept = valid_compare_list(entries, log=logged.append)
748752
kept_ids = [entry["id"] for entry in kept]
749753
skipped_positions = sorted(int(msg.split("compare_list entry ")[1].split(" ")[0]) for msg in logged)
750754

751-
if kept_ids != ["agile", "2024", "numeric_name", "empty_config", "empty_list_config"]:
752-
print("ERROR T27: valid_compare_list() should keep only 'agile', '2024', 'numeric_name', 'empty_config' and 'empty_list_config' (ids as strings), got {}".format(kept_ids))
755+
if kept_ids != ["agile", "2024", "numeric_name", "empty_config", "empty_list_config", "empty_string_config", "empty_dict_config"]:
756+
print("ERROR T27: valid_compare_list() should keep only 'agile', '2024', 'numeric_name' and the four empty-config entries (ids as strings), got {}".format(kept_ids))
753757
failed += 1
754-
elif skipped_positions != [1, 2, 3, 5, 6, 8, 10, 11]:
755-
print("ERROR T27: valid_compare_list() should warn for entries 1, 2, 3, 5, 6, 8 (padded duplicate id), 10 (blank name) and 11 (non-mapping config), got {}".format(logged))
758+
elif skipped_positions != [1, 2, 3, 5, 6, 8, 10, 11, 16, 17]:
759+
print("ERROR T27: valid_compare_list() should warn for entries 1, 2, 3, 5, 6, 8 (padded duplicate id), 10 (blank name), 11, 16 and 17 (unusable config), got {}".format(logged))
756760
failed += 1
757-
elif "config" in kept[3] or "config" in kept[4]:
758-
print("ERROR T27: an empty 'config:' (None or []) should be dropped as no overrides, got {} / {}".format(kept[3], kept[4]))
761+
elif any("config" in entry for entry in kept[3:7]):
762+
print("ERROR T27: an empty 'config:' (None, [], \"\" or {{}}) should be dropped as no overrides, got {}".format(kept[3:7]))
759763
failed += 1
760764
elif kept[4]["name"] != "Empty List Config":
761765
print("ERROR T27: a kept entry's name should be stripped, got {!r}".format(kept[4]["name"]))

0 commit comments

Comments
 (0)