forge: DRY refactor tool_map collect() counting loops into _count_by_tool
This commit is contained in:
1 parent
4ca0befb74
commit
10e9e3ce52
3 files changed
+279
-7
No files matched your search
+19
-7
@@ -210,6 +210,22 @@ def flags_of(repo, rev, name):
|
||||
return out, ""
|
||||
|
||||
|
||||
def _count_by_tool(entries):
|
||||
"""Count probes per tool name (first element of each entry tuple).
|
||||
|
||||
All three probe tables share the same shape for tallying: each entry's
|
||||
first element is the tool name, and the rest is payload the tally ignores.
|
||||
READ_PROBES / NOT_UTF8_PROBES entries are (tool, argv);
|
||||
MALFORMED_PROBES entries are (tool, label, ...). This helper replaces the
|
||||
three identical loops that previously counted each table by hand.
|
||||
"""
|
||||
counts = {}
|
||||
for entry in entries:
|
||||
name = entry[0]
|
||||
counts[name] = counts.get(name, 0) + 1
|
||||
return counts
|
||||
|
||||
|
||||
def collect(repo, rev):
|
||||
"""The whole map as data: one dict per tool plus the gate's legs."""
|
||||
names = tracked_tools(repo, rev)
|
||||
@@ -218,13 +234,9 @@ def collect(repo, rev):
|
||||
sweep = literal_assignments(repo, rev, "tools/selftest_sweep.py",
|
||||
{"READ_PROBES", "NOT_UTF8_PROBES",
|
||||
"MALFORMED_PROBES"})
|
||||
read_n, utf8_n, mal_n = {}, {}, {}
|
||||
for name, _argv in sweep.get("READ_PROBES", ()): # (tool, argv)
|
||||
read_n[name] = read_n.get(name, 0) + 1
|
||||
for name, _argv in sweep.get("NOT_UTF8_PROBES", ()):
|
||||
utf8_n[name] = utf8_n.get(name, 0) + 1
|
||||
for name, *_rest in sweep.get("MALFORMED_PROBES", ()): # (tool, label, ...)
|
||||
mal_n[name] = mal_n.get(name, 0) + 1
|
||||
read_n = _count_by_tool(sweep.get("READ_PROBES", ()))
|
||||
utf8_n = _count_by_tool(sweep.get("NOT_UTF8_PROBES", ()))
|
||||
mal_n = _count_by_tool(sweep.get("MALFORMED_PROBES", ()))
|
||||
legs = literal_assignments(repo, rev, "tools/verify.py", {"LEGS"}).get("LEGS", ())
|
||||
leg_of = {}
|
||||
for leg in legs:
|
||||
|
||||
@@ -0,0 +1,116 @@
|
||||
# forge: tool_map DRY -- collapse three identical counting loops into _count_by_tool
|
||||
|
||||
Target: `tools/tool_map.py` (`collect()`)
|
||||
Lane: Refactor (code review and refactors)
|
||||
Status: DONE -- verified behavior-preserving, drift guard green, output byte-identical.
|
||||
|
||||
## The smell
|
||||
|
||||
`collect()` read three probe tables from `selftest_sweep.py` and tallied each one
|
||||
with a hand-rolled loop, all three doing exactly the same thing -- increment a
|
||||
per-tool name count:
|
||||
|
||||
read_n, utf8_n, mal_n = {}, {}, {}
|
||||
for name, _argv in sweep.get("READ_PROBES", ()): # (tool, argv)
|
||||
read_n[name] = read_n.get(name, 0) + 1
|
||||
for name, _argv in sweep.get("NOT_UTF8_PROBES", ()):
|
||||
utf8_n[name] = utf8_n.get(name, 0) + 1
|
||||
for name, *_rest in sweep.get("MALFORMED_PROBES", ()): # (tool, label, ...)
|
||||
mal_n[name] = mal_n.get(name, 0) + 1
|
||||
|
||||
The only variation was the unpacking shape: READ_PROBES / NOT_UTF8_PROBES entries
|
||||
are 2-tuples `(tool, argv)`, MALFORMED_PROBES entries are 5-tuples
|
||||
`(tool, label, argv, rc, substring)`. In every case the tally only needs
|
||||
`entry[0]`. Three copies of "count the first element per tool name" is the smell.
|
||||
|
||||
## The fix
|
||||
|
||||
One helper, three call sites:
|
||||
|
||||
def _count_by_tool(entries):
|
||||
counts = {}
|
||||
for entry in entries:
|
||||
name = entry[0]
|
||||
counts[name] = counts.get(name, 0) + 1
|
||||
return counts
|
||||
|
||||
read_n = _count_by_tool(sweep.get("READ_PROBES", ()))
|
||||
utf8_n = _count_by_tool(sweep.get("NOT_UTF8_PROBES", ()))
|
||||
mal_n = _count_by_tool(sweep.get("MALFORMED_PROBES", ()))
|
||||
|
||||
No public interface, argument, or output shape changed. The probe tables feed
|
||||
`collect()` only via `read_n.get(name, 0)` / `not_utf8` / `malformed` per row, so
|
||||
two count dicts that agree on every tool produce the same render and the same
|
||||
`--json`. The helper is strictly more uniform than the loops: it indexes `entry[0]`
|
||||
regardless of tuple width, which is exactly what all three loops already did.
|
||||
|
||||
## Evidence
|
||||
|
||||
Three independent verdicts, all green. The verifier is
|
||||
`wizard-work/forge/verify_tool_map_dry_refactor.py`; run it (and the `CHECK:`
|
||||
line below) to reproduce.
|
||||
|
||||
### Check 1 -- the shipped drift guard (end-to-end)
|
||||
|
||||
`python3 tools/tool_map.py --selftest` re-renders `docs/TOOLS.md` at HEAD and
|
||||
compares it to the document on disk; the whole map (purpose, flags, probe counts,
|
||||
gate legs) is byte-derived from the tree. Its currency cell, "HEAD: docs/TOOLS.md
|
||||
is the render of this tree (the drift guard)", is the refactor's real contract:
|
||||
|
||||
selftest: 16 cells, 16 passed, 0 failed
|
||||
(drift guard cell: ok)
|
||||
|
||||
### Check 2 -- direct equivalence, micro level
|
||||
|
||||
Old inline loops vs new `_count_by_tool` on the LIVE probe tables parsed from
|
||||
`selftest_sweep.py` (the exact data `collect()` consumes):
|
||||
|
||||
READ_PROBES: {'append_only_audit.py': 1, 'append_only_owners.py': 1,
|
||||
'census_row_delta.py': 1, 'check_lint.py': 1, 'checkin_check.py': 1,
|
||||
'citation_hash_class.py': 1, 'citation_resolve_check.py': 1,
|
||||
'doc_census_check.py': 1, 'doc_command_check.py': 1, 'landing_check.py': 1,
|
||||
'queue_append_audit.py': 1, 'queue_claim_check.py': 1, 'queue_index.py': 1,
|
||||
'reply_check.py': 1, 'selftest_sweep.py': 1, 'tool_map.py': 1, 'verify.py': 1}
|
||||
NOT_UTF8_PROBES: {'append_only_owners.py': 1, 'census_row_delta.py': 1,
|
||||
'checkin_check.py': 1, 'queue_claim_check.py': 1, 'queue_index.py': 1}
|
||||
MALFORMED_PROBES: {'append_only_audit.py': 1, 'append_only_owners.py': 2,
|
||||
'census_row_delta.py': 2, 'check_lint.py': 2, 'checkin_check.py': 1,
|
||||
'citation_hash_class.py': 1, 'citation_resolve_check.py': 1,
|
||||
'doc_census_check.py': 1, 'doc_command_check.py': 1, 'landing_check.py': 1,
|
||||
'queue_append_audit.py': 1, 'queue_claim_check.py': 1, 'queue_index.py': 1,
|
||||
'reply_check.py': 1, 'selftest_sweep.py': 1, 'tool_map.py': 1, 'verify.py': 1}
|
||||
|
||||
`old_read == new_read` AND `old_utf8 == new_utf8` AND `old_mal == new_mal` --
|
||||
identical, including the MALFORMED_PROBES table where `append_only_owners.py`
|
||||
and `census_row_delta.py` each appear twice (two malformed-input contracts per
|
||||
tool), proving the helper respects the `*_rest` discard shape.
|
||||
|
||||
### Check 3 -- direct equivalence, full output
|
||||
|
||||
`collect(repo, HEAD)` JSON-dumped with `sort_keys=True` from (a) the ORIGINAL
|
||||
module (`git show HEAD:tools/tool_map.py`) and (b) the REFACTORED working-tree
|
||||
module, both reading the SAME tree. The only code that differs between the two
|
||||
is the counting logic, so an identical hash isolates the refactor as the sole
|
||||
variable:
|
||||
|
||||
original.sha256 21653a858a6968448aad7cc3b629c51e0dba3fb3885a21235b2fd0396d54ab41
|
||||
refactored.sha256 21653a858a6968448aad7cc3b629c51e0dba3fb3885a21235b2fd0396d54ab41
|
||||
=> BYTE-IDENTICAL.
|
||||
|
||||
### Check 4 -- the redundancy actually collapsed
|
||||
|
||||
`grep -cE '\[name\] = .*\.get\(name, 0\) \+ 1'` (the tally pattern shared by all
|
||||
three old loops, whose only per-iteration body is `NAME[name] = NAME.get(name, 0) + 1`):
|
||||
|
||||
original (HEAD): 3 occurrences (read_n / utf8_n / mal_n, one per table)
|
||||
refactored: 1 occurrence (inside _count_by_tool, accumulator renamed `counts`)
|
||||
|
||||
No inline tallying loop remains in `collect()`; the only surviving count loop
|
||||
lives in the helper.
|
||||
|
||||
## Exit contract
|
||||
|
||||
Behavior-preserving DRY refactor. `--selftest` 16/16; `collect()` JSON
|
||||
byte-identical between the shipped module at HEAD and the refactored working
|
||||
tree. No output changed. The probe counts flow unchanged into the rendered
|
||||
`docs/TOOLS.md`, which the drift guard confirms stays current.
|
||||
@@ -0,0 +1,144 @@
|
||||
#!/usr/bin/env python3
|
||||
"""verify_tool_map_dry_refactor.py -- equivalence proof for the _count_by_tool DRY.
|
||||
|
||||
The refactor replaced three hand-rolled counting loops in tools/tool_map.py's
|
||||
collect() with a single _count_by_tool() helper. That is behavior-preserving
|
||||
iff collect() on the ORIGINAL (git HEAD:tools/tool_map.py) and collect() on the
|
||||
REFACTORED (working-tree) file, both run against the SAME repo+rev, emit
|
||||
byte-identical data -- because the probe tables they tally are read by the
|
||||
unchanged literal_assignments() from selftest_sweep.py, so the ONLY variable is
|
||||
the counting logic itself.
|
||||
|
||||
Two verdicts are produced, both must be green:
|
||||
|
||||
1. DRIFT GUARD -- `tool_map.py --selftest` exits 0 (its "docs/TOOLS.md is the
|
||||
render of this tree" cell). This is the tool's own shipped contract: the
|
||||
rendered map (which embeds the probe counts) is unchanged.
|
||||
|
||||
2. DIRECT EQUIVALENCE -- collect(repo, HEAD) JSON is byte-identical between
|
||||
the original module and the refactored module (sha256 match), AND the
|
||||
per-tool count dicts from the OLD inline loops vs the NEW helper are
|
||||
identical on the live probe tables.
|
||||
|
||||
Exit code is the verdict: 0 = both green, 1 = any mismatch.
|
||||
"""
|
||||
import hashlib
|
||||
import importlib.util
|
||||
import json
|
||||
import subprocess
|
||||
import sys
|
||||
import tempfile
|
||||
|
||||
REPO = "/root/troupe-standup"
|
||||
TM = f"{REPO}/tools/tool_map.py"
|
||||
SWEET = f"{REPO}/tools/selftest_sweep.py"
|
||||
|
||||
|
||||
def run_selftest():
|
||||
rc = subprocess.run([sys.executable, TM, "--selftest"],
|
||||
capture_output=True, text=True)
|
||||
ok = rc.returncode == 0
|
||||
print(f"[1] selftest (drift guard) rc={rc.returncode} "
|
||||
f"{'PASS' if ok else 'FAIL'}")
|
||||
if not ok:
|
||||
print(" --- selftest stdout ---")
|
||||
print(" " + rc.stdout.rstrip().replace("\n", "\n "))
|
||||
if rc.stderr:
|
||||
print(" --- selftest stderr ---")
|
||||
print(" " + rc.stderr.rstrip().replace("\n", "\n "))
|
||||
return ok
|
||||
|
||||
|
||||
def load_module(path, name):
|
||||
spec = importlib.util.spec_from_file_location(name, path)
|
||||
mod = importlib.util.module_from_spec(spec)
|
||||
spec.loader.exec_module(mod)
|
||||
return mod
|
||||
|
||||
|
||||
def old_counts(sweep):
|
||||
"""The three inline loops, verbatim from the pre-refactor code."""
|
||||
read_n, utf8_n, mal_n = {}, {}, {}
|
||||
for name, _argv in sweep.get("READ_PROBES", ()):
|
||||
read_n[name] = read_n.get(name, 0) + 1
|
||||
for name, _argv in sweep.get("NOT_UTF8_PROBES", ()):
|
||||
utf8_n[name] = utf8_n.get(name, 0) + 1
|
||||
for name, *_rest in sweep.get("MALFORMED_PROBES", ()):
|
||||
mal_n[name] = mal_n.get(name, 0) + 1
|
||||
return read_n, utf8_n, mal_n
|
||||
|
||||
|
||||
def collect_json(mod):
|
||||
data = mod.collect(REPO, "HEAD")
|
||||
return json.dumps(data, sort_keys=True, indent=2)
|
||||
|
||||
|
||||
def main():
|
||||
all_ok = True
|
||||
|
||||
# [2a] micro-equivalence: old loops vs new helper on the LIVE probe tables
|
||||
tm_ref = load_module(TM, "tm_ref")
|
||||
sweep = tm_ref.literal_assignments(
|
||||
REPO, "HEAD", "tools/selftest_sweep.py",
|
||||
{"READ_PROBES", "NOT_UTF8_PROBES", "MALFORMED_PROBES"})
|
||||
old_read, old_utf8, old_mal = old_counts(sweep)
|
||||
new_read = tm_ref._count_by_tool(sweep.get("READ_PROBES", ()))
|
||||
new_utf8 = tm_ref._count_by_tool(sweep.get("NOT_UTF8_PROBES", ()))
|
||||
new_mal = tm_ref._count_by_tool(sweep.get("MALFORMED_PROBES", ()))
|
||||
micro_ok = (old_read == new_read and old_utf8 == new_utf8
|
||||
and old_mal == new_mal)
|
||||
print(f"[2a] micro: old inline loops == _count_by_tool on live tables "
|
||||
f"{'PASS' if micro_ok else 'FAIL'}")
|
||||
if not micro_ok:
|
||||
all_ok = False
|
||||
for label, o, n in [("READ", old_read, new_read),
|
||||
("NOT_UTF8", old_utf8, new_utf8),
|
||||
("MALFORMED", old_mal, new_mal)]:
|
||||
if o != n:
|
||||
print(f" {label} MISMATCH old={o} new={n}")
|
||||
|
||||
# [2b] full collect() equivalence: original (git HEAD) vs refactored
|
||||
orig_src = subprocess.run(
|
||||
["git", "-C", REPO, "show", "HEAD:tools/tool_map.py"],
|
||||
capture_output=True, text=True).stdout
|
||||
if not orig_src:
|
||||
print("[2b] could not read HEAD:tools/tool_map.py -- FAIL")
|
||||
all_ok = False
|
||||
else:
|
||||
with tempfile.NamedTemporaryFile("w", suffix="_tm_orig.py",
|
||||
delete=False) as fh:
|
||||
fh.write(orig_src)
|
||||
orig_path = fh.name
|
||||
tm_orig = load_module(orig_path, "tm_orig")
|
||||
j_orig = collect_json(tm_orig)
|
||||
j_ref = collect_json(tm_ref)
|
||||
h_orig = hashlib.sha256(j_orig.encode()).hexdigest()
|
||||
h_ref = hashlib.sha256(j_ref.encode()).hexdigest()
|
||||
full_ok = j_orig == j_ref and h_orig == h_ref
|
||||
print(f"[2b] collect() JSON original.sha256={h_orig[:16]}… "
|
||||
f"refactored.sha256={h_ref[:16]}… "
|
||||
f"{'PASS' if full_ok else 'FAIL'}")
|
||||
if not full_ok:
|
||||
all_ok = False
|
||||
# show first diff line for a targeted signal
|
||||
a, b = j_orig.splitlines(), j_ref.splitlines()
|
||||
for i in range(max(len(a), len(b))):
|
||||
ga = a[i] if i < len(a) else "<end>"
|
||||
gb = b[i] if i < len(b) else "<end>"
|
||||
if ga != gb:
|
||||
print(f" first diff at json line {i+1}:")
|
||||
print(f" original: {ga}")
|
||||
print(f" refactored: {gb}")
|
||||
break
|
||||
|
||||
selftest_ok = run_selftest()
|
||||
if not selftest_ok:
|
||||
all_ok = False
|
||||
|
||||
print("\nVERDICT:", "ALL GREEN -- behavior preserved" if all_ok
|
||||
else "MISMATCH -- behavior NOT preserved")
|
||||
sys.exit(0 if all_ok else 1)
|
||||
|
||||
|
||||
if __name__ == "__main__":
|
||||
main()
|
||||
Reference in new issue
Block a user