forge: doc_command_check report() tally loop -> Counter (DRY, behavior-preserving)
This commit is contained in:
1 parent
223d9410ad
commit
86950bc00a
3 files changed
+290
-3
No files matched your search
@@ -69,6 +69,7 @@ import re
|
||||
import subprocess
|
||||
import sys
|
||||
import tempfile
|
||||
from collections import Counter
|
||||
|
||||
# A documented command: `python3 <path>.py <args...>`. The path may be written
|
||||
# as `tools/x.py` from the repo root or as `x.py` from inside `tools/`, which
|
||||
@@ -228,9 +229,11 @@ def report(rep, out=sys.stdout):
|
||||
miss = (" missing=%s" % (r["missing"],)) if r["missing"] else ""
|
||||
print("%-18s %-30s -> %-26s %s%s"
|
||||
% (r["verdict"], r["doc"], r["target"], r["detail"], miss), file=out)
|
||||
counts = {}
|
||||
for r in rep["rows"]:
|
||||
counts[r["verdict"]] = counts.get(r["verdict"], 0) + 1
|
||||
# `collections.Counter` replaces the hand-rolled tally loop: every entry's
|
||||
# verdict is counted exactly as `counts[k] = counts.get(k, 0) + 1` did, and
|
||||
# `sorted(counts)` still orders the keys, so the rendered summary is
|
||||
# byte-identical -- this is a deletion of a duplicate loop, not a rewrite.
|
||||
counts = Counter(r["verdict"] for r in rep["rows"])
|
||||
print("doc commands at %s: %d command(s) over %d oracle(s) -- %s"
|
||||
% (rep["rev"], len(rep["rows"]), rep["oracles"],
|
||||
", ".join("%s %d" % (k, counts[k]) for k in sorted(counts)) or "none"))
|
||||
|
||||
@@ -0,0 +1,80 @@
|
||||
# forge: doc_command_check report() -- replace hand-rolled tally loop with Counter (DRY)
|
||||
|
||||
Target: `tools/doc_command_check.py`
|
||||
Lane: Refactor (code review and refactors)
|
||||
Status: DONE -- verified equivalent, behavior preserved.
|
||||
|
||||
## The smell
|
||||
|
||||
`report()` counted verdicts with a hand-rolled loop:
|
||||
|
||||
counts = {}
|
||||
for r in rep["rows"]:
|
||||
counts[r["verdict"]] = counts.get(r["verdict"], 0) + 1
|
||||
|
||||
That is the exact same tally shape `tool_map.collect()` had before the
|
||||
tool_map DRY -- a `counts.get(name, 0) + 1` loop the house already replaced
|
||||
with a single-counting abstraction. `report()` is the lone remaining instance
|
||||
in `doc_command_check.py`, and it is the only place in the file that counts by
|
||||
hand.
|
||||
|
||||
## The fix (behavior-preserving)
|
||||
|
||||
Replace the loop with `collections.Counter`, the count dict is consumed only
|
||||
as `sorted(counts)` + `counts[k]`, both of which Counter satisfies identically
|
||||
(Counter is a `dict` subclass; only keys that occurred appear; a missing key
|
||||
returns `0` but no key is ever missing because the loop is over its own keys).
|
||||
|
||||
- counts = {}
|
||||
- for r in rep["rows"]:
|
||||
- counts[r["verdict"]] = counts.get(r["verdict"], 0) + 1
|
||||
+ counts = Counter(r["verdict"] for r in rep["rows"])
|
||||
|
||||
`report()` is the ONLY function touched. `census()`, `documented_commands()`,
|
||||
`resolve_target()`, `help_options()`, and `selftest()` are byte-identical to
|
||||
their pre-refactor form. The full module `git diff` is two hunks: the
|
||||
`from collections import Counter` import and the three-line collapse above.
|
||||
|
||||
## Evidence
|
||||
|
||||
`wizard-work/forge/verify_doc_command_report_dry.py` exercises three verdicts.
|
||||
All green (exit 0):
|
||||
|
||||
[1] SELFTEST DRIFT GUARD -- `doc_command_check.py --selftest` on BOTH the
|
||||
original (git HEAD blob, run under its real name) and the refactored
|
||||
(working tree):
|
||||
original selftest rc=0 sha=36f0f40814b819f162302a56cd699682... PASS
|
||||
refactored selftest rc=0 sha=36f0f40814b819f162302a56cd699682... PASS
|
||||
stdout is byte-identical (sha256 match) to the captured baseline and to
|
||||
each other -- 15 cells, 15 passed, 0 failed. (The original is run under
|
||||
the name `doc_command_check.py` because its selftest is self-referential
|
||||
via `__file__`'s basename; a randomly-named temp file falsely fails those
|
||||
cells for a harness reason, which the verify script avoids.)
|
||||
|
||||
[2a] MICRO EQUIVALENCE -- on the LIVE census at HEAD, the old inline loop and
|
||||
the new Counter() produce identical count dicts:
|
||||
live tally: old loop == Counter PASS counts={'OK': 58}
|
||||
|
||||
[2b] report() byte-identity on synthetic reps covering every branch the
|
||||
function cares about:
|
||||
report(synthetic [mixed]) identical PASS (FLAG_NOT_ACCEPTED/NO_ORACLE/OK/TOOL_MISSING)
|
||||
report(synthetic [all_ok (none fallback)]) PASS (the `-- none` arm)
|
||||
report(synthetic [empty]) PASS (empty rows -> "0 command(s)... none")
|
||||
|
||||
[3] REPORT OUTPUT DRIFT -- report() on the live census(rep), original vs
|
||||
refactored, byte-identical:
|
||||
report(live census) identical PASS bytes=63
|
||||
|
||||
Note: `report()`'s summary `print()` writes to `sys.stdout` (not the `out=`
|
||||
parameter the fault-row prints use), so the verify script redirects stdout
|
||||
into the same buffer as `out` to capture the full text -- including the summary
|
||||
line whose counts are what this refactor changes. The capture is why [3]
|
||||
measures 63 bytes rather than 0.
|
||||
|
||||
## Exit contract
|
||||
|
||||
Behavior-preserving refactor. rc 0 on both the original and refactored
|
||||
`--selftest`, stdout byte-identical (sha256 match), and `report()` output
|
||||
byte-identical on both synthetic and live census data. No public interface,
|
||||
argument, or output format changed; the summary line ordering
|
||||
(`sorted(counts)`) and the `-- none` fallback are unchanged.
|
||||
@@ -0,0 +1,204 @@
|
||||
#!/usr/bin/env python3
|
||||
"""verify_doc_command_report_dry.py -- equivalence proof for the report() DRY.
|
||||
|
||||
The refactor replaced the hand-rolled tally loop in tools/doc_command_check.py's
|
||||
report() with collections.Counter:
|
||||
|
||||
- counts = {}
|
||||
- for r in rep["rows"]:
|
||||
- counts[r["verdict"]] = counts.get(r["verdict"], 0) + 1
|
||||
+ counts = Counter(r["verdict"] for r in rep["rows"])
|
||||
|
||||
That is behavior-preserving iff report() emits byte-identical text before and
|
||||
after, because report() is the ONLY function touched, and every other path
|
||||
(census(), documented_commands(), resolve_target(), help_options()) is unchanged.
|
||||
|
||||
Three verdicts, all must be green:
|
||||
|
||||
1. SELFTEST DRIFT GUARD -- `doc_command_check.py --selftest` exits 0 on BOTH
|
||||
the original (git HEAD blob, run under its real name) and the refactored
|
||||
(working tree), and their stdout is byte-identical (sha256 match) to the
|
||||
captured baseline. The selftest's 15-cell fixture corpus exercises the
|
||||
counting branch through report().
|
||||
|
||||
2. MICRO EQUIVALENCE -- on the LIVE census at HEAD, the old inline tally loop
|
||||
and the new Counter() produce identical count dicts, AND report() on every
|
||||
synthetic rep (mixed verdicts, all-OK, empty) is byte-identical between the
|
||||
original and refactored modules.
|
||||
|
||||
3. REPORT OUTPUT DRIFT -- report() on the live census(rep) is byte-identical
|
||||
between original and refactored: the changed function's rendered text did
|
||||
not move by one byte.
|
||||
|
||||
Exit code is the verdict: 0 = all green, 1 = any mismatch.
|
||||
"""
|
||||
import hashlib
|
||||
import importlib.util
|
||||
import io
|
||||
import os
|
||||
import subprocess
|
||||
import sys
|
||||
import tempfile
|
||||
from collections import Counter
|
||||
|
||||
REPO = "/root/troupe-standup"
|
||||
DCC = f"{REPO}/tools/doc_command_check.py"
|
||||
# Captured from the original (pre-refactor) source at git HEAD, run under its
|
||||
# real name `doc_command_check.py`:
|
||||
# python3 tools/doc_command_check.py --selftest
|
||||
BASELINE_SELFTEST_SHA = "36f0f40814b819f162302a56cd6996820e647ffba255f4f821ce19dc9f3815d2"
|
||||
|
||||
|
||||
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 run_selftest(src_path, label):
|
||||
rc = subprocess.run([sys.executable, src_path, "--selftest"],
|
||||
capture_output=True, text=True)
|
||||
out = rc.stdout or ""
|
||||
sha = hashlib.sha256(out.encode()).hexdigest()
|
||||
ok = rc.returncode == 0 and sha == BASELINE_SELFTEST_SHA
|
||||
print(f"[1] {label:10s} selftest rc={rc.returncode} sha={sha[:16]}... "
|
||||
f"{'PASS' if ok else 'FAIL'}")
|
||||
if not ok:
|
||||
print(f" expected sha={BASELINE_SELFTEST_SHA[:16]}...")
|
||||
if rc.stderr:
|
||||
print(" --- stderr ---")
|
||||
print(" " + rc.stderr.rstrip().replace("\n", "\n "))
|
||||
return ok
|
||||
|
||||
|
||||
def old_tally(rep):
|
||||
"""The three-line loop, verbatim from the pre-refactor source."""
|
||||
counts = {}
|
||||
for r in rep["rows"]:
|
||||
counts[r["verdict"]] = counts.get(r["verdict"], 0) + 1
|
||||
return counts
|
||||
|
||||
|
||||
def report_text(mod, rep):
|
||||
"""All of report()'s text. Fault-row prints honour `out=`, but the summary
|
||||
print() hard-wires sys.stdout, so BOTH are captured by redirecting stdout
|
||||
into the same buffer handed in as `out`."""
|
||||
buf = io.StringIO()
|
||||
old = sys.stdout
|
||||
sys.stdout = buf
|
||||
try:
|
||||
mod.report(rep, out=buf)
|
||||
finally:
|
||||
sys.stdout = old
|
||||
return buf.getvalue()
|
||||
|
||||
|
||||
def synthetic_reps():
|
||||
"""Cover every branch report() cares about: fault rows + multi-key counts."""
|
||||
mixed = {
|
||||
"rev": "HEAD", "oracles": 3, "rows": [
|
||||
{"doc": "tools/a.py", "target": "tools/a.py", "verdict": "OK",
|
||||
"missing": [], "detail": ""},
|
||||
{"doc": "tools/a.py", "target": "gone.py", "verdict": "TOOL_MISSING",
|
||||
"missing": [], "detail": "no such file at HEAD"},
|
||||
{"doc": "tools/a.py", "target": "tools/a.py",
|
||||
"verdict": "FLAG_NOT_ACCEPTED", "missing": ["--gone"], "detail": ""},
|
||||
{"doc": "tools/b.py", "target": "tools/b.py", "verdict": "NO_ORACLE",
|
||||
"missing": [], "detail": "no --help output"},
|
||||
{"doc": "tools/a.py", "target": "tools/a.py", "verdict": "OK",
|
||||
"missing": [], "detail": ""},
|
||||
],
|
||||
}
|
||||
all_ok = {
|
||||
"rev": "HEAD", "oracles": 0, "rows": [
|
||||
{"doc": "tools/a.py", "target": "tools/a.py", "verdict": "OK",
|
||||
"missing": [], "detail": ""},
|
||||
{"doc": "tools/a.py", "target": "tools/b.py", "verdict": "OK",
|
||||
"missing": [], "detail": ""},
|
||||
],
|
||||
}
|
||||
empty = {"rev": "HEAD", "oracles": 0, "rows": []}
|
||||
return [("mixed", mixed), ("all_ok (none fallback)", all_ok),
|
||||
("empty", empty)]
|
||||
|
||||
|
||||
def main():
|
||||
all_ok = True
|
||||
|
||||
# Original source blob at HEAD, materialized under its real name. The
|
||||
# original's own selftest is self-referential via `__file__`'s basename
|
||||
# (its docstring-against-itself cells), so the temp file MUST be named
|
||||
# doc_command_check.py or those cells fail for a harness reason, not a
|
||||
# behavior one.
|
||||
orig_src = subprocess.run(
|
||||
["git", "-C", REPO, "show", "HEAD:tools/doc_command_check.py"],
|
||||
capture_output=True, text=True).stdout
|
||||
if not orig_src:
|
||||
print("[1] could not read HEAD:tools/doc_command_check.py -- FAIL")
|
||||
return 1
|
||||
orig_dir = tempfile.mkdtemp(prefix="dcc_orig_")
|
||||
orig_path = os.path.join(orig_dir, "doc_command_check.py")
|
||||
with open(orig_path, "w") as fh:
|
||||
fh.write(orig_src)
|
||||
os.chmod(orig_path, 0o755)
|
||||
|
||||
tm_orig = load_module(orig_path, "dcc_orig")
|
||||
tm_ref = load_module(DCC, "dcc_ref")
|
||||
|
||||
# [1] selftest drift guard: both versions, byte-identical to baseline.
|
||||
o_ok = run_selftest(orig_path, "original")
|
||||
r_ok = run_selftest(DCC, "refactored")
|
||||
selftest_ok = o_ok and r_ok
|
||||
all_ok = all_ok and selftest_ok
|
||||
|
||||
# [2a] micro equivalence: live census tally, old loop == new Counter.
|
||||
live = tm_ref.census(REPO, "HEAD")
|
||||
old_counts = old_tally(live)
|
||||
new_counts = Counter(r["verdict"] for r in live["rows"])
|
||||
micro_ok = old_counts == new_counts
|
||||
print(f"[2a] live tally: old loop == Counter "
|
||||
f"{'PASS' if micro_ok else 'FAIL'} counts={dict(new_counts)}")
|
||||
if not micro_ok:
|
||||
all_ok = False
|
||||
print(f" old={dict(old_counts)} new={dict(new_counts)}")
|
||||
|
||||
# [2b] report() byte-identity on synthetic reps (every verdict branch).
|
||||
for label, rep in synthetic_reps():
|
||||
a = report_text(tm_orig, rep)
|
||||
b = report_text(tm_ref, rep)
|
||||
eq = a == b
|
||||
print(f"[2b] report(synthetic [{label}]) identical "
|
||||
f"{'PASS' if eq else 'FAIL'}")
|
||||
if not eq:
|
||||
all_ok = False
|
||||
la, lb = a.splitlines(), b.splitlines()
|
||||
for i in range(max(len(la), len(lb))):
|
||||
ga = la[i] if i < len(la) else "<end>"
|
||||
gb = lb[i] if i < len(lb) else "<end>"
|
||||
if ga != gb:
|
||||
print(f" diff at line {i+1}: orig={ga!r} ref={gb!r}")
|
||||
break
|
||||
|
||||
# [3] report() byte-identity on the LIVE census(rep): end-to-end of the
|
||||
# touched function against the real tree.
|
||||
ro = report_text(tm_orig, live)
|
||||
rr = report_text(tm_ref, live)
|
||||
live_ok = ro == rr
|
||||
print(f"[3] report(live census) identical "
|
||||
f"{'PASS' if live_ok else 'FAIL'} bytes={len(rr)}")
|
||||
if not live_ok:
|
||||
all_ok = False
|
||||
print(" --- original (first 600 chars) ---")
|
||||
print(" " + ro[:600].replace("\n", "\n "))
|
||||
print(" --- refactored (first 600 chars) ---")
|
||||
print(" " + rr[:600].replace("\n", "\n "))
|
||||
|
||||
print("\nVERDICT:",
|
||||
"ALL GREEN -- report() 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