forge: check_lint DRY refactor - extract _write_json_file from 2 duplicated json.dump blocks, verify behavior-preservation (8/8 checks pass)
This commit is contained in:
forge committed
1 parent
a960d5a700
commit
6880cab214
2 files changed
+337
No files matched your search
@@ -0,0 +1,121 @@
|
||||
# check_lint.py DRY refactor verification (forge lane: Refactor)
|
||||
|
||||
## The shipped artifact
|
||||
`tools/check_lint.py` -- the per-tool lint/check runner. Its `--selftest`
|
||||
runs a 37-cell census (fixtures + portability + `--json` + `--check` arms); the
|
||||
`--check --json` path writes a classification dict to disk; the `report()`
|
||||
`--json` path writes the full run summary to disk.
|
||||
|
||||
## The smell (what the refactor fixes)
|
||||
JSON file-writing was inlined at two sites with the SAME try/except block,
|
||||
copied verbatim:
|
||||
|
||||
1. `report()` --json path (writes the full run summary to `as_json`)
|
||||
2. `main()` `--check --json` branch (writes the single-classification dict to `a.json`)
|
||||
|
||||
Both blocks were byte-identical in structure:
|
||||
|
||||
```python
|
||||
try:
|
||||
with open(<path>, "w", encoding="utf-8") as fh:
|
||||
json.dump(<obj>, fh, indent=2)
|
||||
fh.write("\n")
|
||||
except OSError as exc:
|
||||
print("cannot write --json " + <path> + ": " + str(exc), file=sys.stderr)
|
||||
return 2
|
||||
```
|
||||
|
||||
Two copies of one 7-line statement is the canonical DRY smell; each new JSON
|
||||
output site would be one more freehand copy away from drifting. The block is
|
||||
context-free (no closure over loop state), making it a safe extraction target.
|
||||
|
||||
## The refactor (working tree, uncommitted)
|
||||
A single module-level helper replaces both inlined blocks:
|
||||
|
||||
```python
|
||||
def _write_json_file(path: str, obj) -> int:
|
||||
"""Write obj as indented JSON to path. Returns 0 on success, 2 on OSError.
|
||||
|
||||
Extracted from the two sites that previously hand-duplicated the
|
||||
open/dump/write/newline/except block: report() and the --check/--json
|
||||
branch in main(). Both used indent=2 with default sort_keys=False.
|
||||
"""
|
||||
try:
|
||||
with open(path, "w", encoding="utf-8") as fh:
|
||||
json.dump(obj, fh, indent=2, sort_keys=False)
|
||||
fh.write("\n")
|
||||
except OSError as exc:
|
||||
print("cannot write --json " + path + ": " + str(exc), file=sys.stderr)
|
||||
return 2
|
||||
return 0
|
||||
```
|
||||
|
||||
Both call sites are replaced by:
|
||||
|
||||
```python
|
||||
rc = _write_json_file(<path>, <obj>)
|
||||
if rc:
|
||||
return rc
|
||||
```
|
||||
|
||||
Same open mode, same `indent=2`, same `sort_keys=False` (the default
|
||||
`--check` branch relied on), same `"\n"` terminator, same error message, same
|
||||
rc 2. Behavior is unchanged; the duplicated statement is collapsed into one
|
||||
definition.
|
||||
|
||||
## ORIGINAL vs REFACTORED
|
||||
- ORIGINAL = `git show HEAD:tools/check_lint.py` (the committed pre-refactor
|
||||
source). Anchored on HEAD because the refactor is uncommitted; this pin is
|
||||
robust to any number of commits landing before the run.
|
||||
- REFACTORED = working tree.
|
||||
|
||||
## Verifier output (8 checks, all pass, exit 0)
|
||||
```
|
||||
[ok ] [1] selftest: original (HEAD) 37/37 rc 0
|
||||
[ok ] [2] selftest: refactored (working tree) 37/37 rc 0
|
||||
[ok ] [3] selftest: stdout BYTE-IDENTICAL (original == refactored)
|
||||
[ok ] [4] code-level: refactored HAS _write_json_file; original NONE
|
||||
[ok ] [5] code-level: json.dump( count 2 in original, 1 in refactored
|
||||
[ok ] [6] non-vacuity: live run produced a real 37-cell census
|
||||
[ok ] [7] --check --json: stdout + JSON file BYTE-IDENTICAL (rc match)
|
||||
[ok ] [8] --check --json unwritable path: both return rc 2
|
||||
|
||||
verify: 8 checks, 8 passed, 0 failed
|
||||
```
|
||||
|
||||
## Legs
|
||||
- [1-2] **selftest parity.** Both original (HEAD) and refactored (working tree)
|
||||
pass 37/37 cells with rc 0. The selftest is the comprehensive equivalence
|
||||
surface: it drives `--check` with a `CATCH_ALL`-class fault, exercises the
|
||||
`--json` write path, the `--fail-on-fault` rc-1 arm, portability cells, and
|
||||
the full fixture census.
|
||||
- [3] **byte-identical stdout.** Every character of both selftests matches --
|
||||
proving no cell's verdict, detail line, or formatted output changed.
|
||||
- [4-5] **non-faculty code-level checks.** The helper
|
||||
(`def _write_json_file(`) exists only in the refactored source; the two
|
||||
inlined `json.dump(...)` call sites exist only in the original. Together
|
||||
[4]+[5] prove the smell genuinely existed (count 2) and was genuinely
|
||||
collapsed (count 1) -- not a no-op refactor.
|
||||
- [6] **non-vacuity.** The live `--selftest --verbose` run produced a real
|
||||
37-cell census (`cell 37:` present, `cell ` count >= 37), not a stub.
|
||||
- [7] **`--check --json` equivalence.** On a real faulting check
|
||||
(`CHECK: python3 x.py || true`, `--fail-on-fault`), both versions produce
|
||||
BYTE-IDENTICAL stdout AND a BYTE-IDENTICAL JSON file on disk, with a
|
||||
matching rc (the `--fail-on-fault` -> rc 1 arm).
|
||||
- [8] **error-path parity.** On an unwritable `--json` path
|
||||
(`/nonexistent/does/not/exist.json`), both versions return rc 2 (the
|
||||
`OSError -> return 2` branch is preserved through the extraction).
|
||||
|
||||
## Non-tautology proof
|
||||
Against the pinned original (HEAD), the refactor markers are absent:
|
||||
`def _write_json_file` is NOT in HEAD, and there are TWO `json.dump(` call
|
||||
sites in HEAD but ONE in the refactored source. The helper's body is a literal
|
||||
lift of the original try/except (same `indent=2`, same `sort_keys=False`,
|
||||
same `"\n"`, same message), so it behaves identically whether inlined or
|
||||
extracted.
|
||||
|
||||
## Artifacts
|
||||
- Refactored tool: `tools/check_lint.py` (working tree)
|
||||
- Equivalence verifier: `wizard-work/forge/verify_check_lint_dry_refactor.py`
|
||||
- Post-check CHECK: `python3 /root/troupe-standup/wizard-work/forge/verify_check_lint_dry_refactor.py`
|
||||
- Report: this file
|
||||
@@ -0,0 +1,216 @@
|
||||
#!/usr/bin/env python3
|
||||
"""verify_check_lint_dry_refactor.py
|
||||
|
||||
Equivalence proof for a behavior-preserving DRY refactor in tools/check_lint.py
|
||||
(forge lane: Refactor).
|
||||
|
||||
THE SMELL
|
||||
check_lint.py wrote JSON to disk in two places with the SAME try/except block,
|
||||
hand-duplicated verbatim:
|
||||
|
||||
1. report() --json path (writing the full run summary)
|
||||
2. main() --check --json branch (writing the single-classification dict)
|
||||
|
||||
Both blocks were identical in structure:
|
||||
try:
|
||||
with open(<path>, "w", encoding="utf-8") as fh:
|
||||
json.dump(<obj>, fh, indent=2[, sort_keys=False])
|
||||
fh.write("\n")
|
||||
except OSError as exc:
|
||||
print("cannot write --json " + <path> + ": " + str(exc), file=sys.stderr)
|
||||
return 2
|
||||
|
||||
Two copies of one 7-line statement is the canonical DRY smell; each new JSON
|
||||
output site would be one more freehand copy away from drifting.
|
||||
|
||||
THE REFACTOR (committed? NO -- working tree only)
|
||||
A single module-level helper replaces both inlined blocks:
|
||||
|
||||
def _write_json_file(path: str, obj) -> int:
|
||||
'Write obj as indented JSON to path. Returns 0 on success, 2 on OSError.'
|
||||
try:
|
||||
with open(path, "w", encoding="utf-8") as fh:
|
||||
json.dump(obj, fh, indent=2, sort_keys=False)
|
||||
fh.write("\\n")
|
||||
except OSError as exc:
|
||||
print("cannot write --json " + path + ": " + str(exc), file=sys.stderr)
|
||||
return 2
|
||||
return 0
|
||||
|
||||
Both call sites are replaced by:
|
||||
rc = _write_json_file(<path>, <obj>)
|
||||
if rc:
|
||||
return rc
|
||||
|
||||
Same open mode, same indent=2, same sort_keys=False (the default --check branch
|
||||
relied on), same "\\n" terminator, same error message, same rc 2.
|
||||
|
||||
WHAT THIS VERIFIER PROVES
|
||||
The refactor is behavior-preserving, on faithful copies of each version run in
|
||||
an isolated temp file (self-contained selftest; no sibling tools needed).
|
||||
|
||||
LEGS
|
||||
[1] selftest: original (HEAD) 37/37 rc 0
|
||||
[2] selftest: refactored (working tree) 37/37 rc 0
|
||||
[3] selftest: stdout is BYTE-IDENTICAL (original == refactored)
|
||||
[4] code-level: refactored HAS the helper _write_json_file; original has none
|
||||
[5] code-level: json.dump( count is 2 in original, 1 in refactored
|
||||
(proves the smell existed and was collapsed into the helper)
|
||||
[6] non-vacuity: the live run produced a real 37-cell census (not a stub)
|
||||
[7] --check --json: stdout BYTE-IDENTICAL + JSON file content BYTE-IDENTICAL
|
||||
on both versions ( CATCH_ALL case with --fail-on-fault -> rc 1, JSON written)
|
||||
[8] --check --json unwritable path: both return rc 2 (OSError path preserved)
|
||||
|
||||
Exit 0 iff all checks pass, else 1.
|
||||
|
||||
NOTE ON PROVENANCE
|
||||
The refactor is an uncommitted working-tree change, so the genuine
|
||||
pre-refactor original is `git show HEAD:tools/check_lint.py` (HEAD is the
|
||||
committed original since the refactor has not been committed). That pin is
|
||||
robust to any number of commits landing before the run.
|
||||
"""
|
||||
import json
|
||||
import os
|
||||
import subprocess
|
||||
import sys
|
||||
import tempfile
|
||||
|
||||
REPO = "/root/troupe-standup"
|
||||
TOOL = "tools/check_lint.py"
|
||||
SUMMARY = "selftest: 37 cells, 37 passed, 0 failed"
|
||||
TMPDIR = os.environ.get("TMPDIR", tempfile.gettempdir())
|
||||
|
||||
|
||||
class Mismatch(Exception):
|
||||
pass
|
||||
|
||||
|
||||
def orig_text():
|
||||
"""The committed pre-refactor source (HEAD of tools/check_lint.py)."""
|
||||
proc = subprocess.run(["git", "-C", REPO, "show", "HEAD:" + TOOL],
|
||||
capture_output=True, text=True, timeout=60)
|
||||
if proc.returncode != 0:
|
||||
raise Mismatch("cannot read HEAD:" + TOOL + ": " + proc.stderr)
|
||||
return proc.stdout
|
||||
|
||||
|
||||
def ref_text():
|
||||
"""The working-tree (refactored) source."""
|
||||
with open(os.path.join(REPO, TOOL), encoding="utf-8") as fh:
|
||||
return fh.read()
|
||||
|
||||
|
||||
def run_script(source_text, args):
|
||||
"""Write source_text to an isolated temp file and run it with args.
|
||||
Returns (rc, stdout, stderr)."""
|
||||
fd, path = tempfile.mkstemp(suffix=".py", dir=TMPDIR)
|
||||
try:
|
||||
with os.fdopen(fd, "w") as fh:
|
||||
fh.write(source_text)
|
||||
proc = subprocess.run([sys.executable, path] + args,
|
||||
capture_output=True, text=True, timeout=120)
|
||||
return proc.returncode, proc.stdout, proc.stderr
|
||||
finally:
|
||||
os.unlink(path)
|
||||
|
||||
|
||||
def count_json_dump(text):
|
||||
"""Count direct json.dump( calls in source text (file-writing, not json.dumps)."""
|
||||
return text.count("json.dump(")
|
||||
|
||||
|
||||
def has_helper(text):
|
||||
return "def _write_json_file(" in text
|
||||
|
||||
|
||||
def main():
|
||||
cells, failed = [], []
|
||||
|
||||
def check(label, ok, detail=""):
|
||||
cells.append(label)
|
||||
if not ok:
|
||||
failed.append("%s%s" % (label, (": " + detail) if detail else ""))
|
||||
print(" [%s] %s%s"
|
||||
% ("ok " if ok else "FAIL", label,
|
||||
(" [%s]" % detail) if detail and not ok else ""))
|
||||
|
||||
orig = orig_text()
|
||||
ref = ref_text()
|
||||
|
||||
# ---- [1-3] selftest equivalence ---------------------------------------
|
||||
rc_o, out_o, err_o = run_script(orig, ["--selftest"])
|
||||
rc_r, out_r, err_r = run_script(ref, ["--selftest"])
|
||||
|
||||
check("[1] selftest: original (HEAD) 37/37 rc 0",
|
||||
rc_o == 0 and SUMMARY in out_o,
|
||||
"rc=%d %s" % (rc_o, (out_o + err_o)[-240:]))
|
||||
check("[2] selftest: refactored (working tree) 37/37 rc 0",
|
||||
rc_r == 0 and SUMMARY in out_r,
|
||||
"rc=%d %s" % (rc_r, (out_r + err_r)[-240:]))
|
||||
check("[3] selftest: stdout BYTE-IDENTICAL (original == refactored)",
|
||||
out_o == out_r,
|
||||
"len_o=%d len_r=%d" % (len(out_o), len(out_r)))
|
||||
|
||||
# ---- [4-5] code-level: smell existed and was removed --------------------
|
||||
check("[4] code-level: refactored HAS _write_json_file; original NONE",
|
||||
has_helper(ref) and not has_helper(orig),
|
||||
"ref=%s orig=%s" % (has_helper(ref), has_helper(orig)))
|
||||
check("[5] code-level: json.dump( count 2 in original, 1 in refactored",
|
||||
count_json_dump(orig) == 2 and count_json_dump(ref) == 1,
|
||||
"orig=%d ref=%d" % (count_json_dump(orig), count_json_dump(ref)))
|
||||
|
||||
# ---- [6] non-vacuity ---------------------------------------------------
|
||||
# Run --selftest --verbose: a real census prints each cell line, proving
|
||||
# the fixtures are actually iterated and classified (not a stub that only
|
||||
# prints the summary).
|
||||
rc_v, out_v, _ = run_script(ref, ["--selftest", "--verbose"])
|
||||
check("[6] non-vacuity: live run produced a real 37-cell census",
|
||||
rc_v == 0 and "cell 37:" in out_v and SUMMARY in out_v
|
||||
and out_v.count("cell ") >= 37,
|
||||
"rc=%d cells_seen=%d" % (rc_v, out_v.count("cell ")))
|
||||
|
||||
# ---- [7] --check --json equivalence (CATCH_ALL + faults + JSON file) --
|
||||
json_path_o = os.path.join(TMPDIR, "cl_orig.json")
|
||||
json_path_r = os.path.join(TMPDIR, "cl_ref.json")
|
||||
try:
|
||||
args = ["--check", "CHECK: python3 x.py || true", "--json", None,
|
||||
"--fail-on-fault"]
|
||||
rc_o2, out_o2, err_o2 = run_script(orig, [a if a is not None else json_path_o
|
||||
for a in args])
|
||||
rc_r2, out_r2, err_r2 = run_script(ref, [a if a is not None else json_path_r
|
||||
for a in args])
|
||||
with open(json_path_o) as fh:
|
||||
json_o = fh.read()
|
||||
with open(json_path_r) as fh:
|
||||
json_r = fh.read()
|
||||
check("[7] --check --json: stdout + JSON file BYTE-IDENTICAL (rc match)",
|
||||
rc_o2 == rc_r2 and out_o2 == out_r2 and json_o == json_r,
|
||||
"rc_o=%d rc_r=%d stdout_match=%s json_match=%s" %
|
||||
(rc_o2, rc_r2, out_o2 == out_r2, json_o == json_r))
|
||||
finally:
|
||||
for p in (json_path_o, json_path_r):
|
||||
if os.path.exists(p):
|
||||
os.unlink(p)
|
||||
|
||||
# ---- [8] --check --json unwritable path -> rc 2 both -------------------
|
||||
rc_o3, _, _ = run_script(orig, ["--check", "CHECK: true", "--json",
|
||||
"/nonexistent/does/not/exist.json"])
|
||||
rc_r3, _, _ = run_script(ref, ["--check", "CHECK: true", "--json",
|
||||
"/nonexistent/does/not/exist.json"])
|
||||
check("[8] --check --json unwritable path: both return rc 2",
|
||||
rc_o3 == 2 and rc_r3 == 2,
|
||||
"rc_o=%d rc_r=%d" % (rc_o3, rc_r3))
|
||||
|
||||
print("\nverify: %d checks, %d passed, %d failed"
|
||||
% (len(cells), len(cells) - len(failed), len(failed)))
|
||||
for f in failed:
|
||||
print(" FAIL %s" % f)
|
||||
return 1 if failed else 0
|
||||
|
||||
|
||||
if __name__ == "__main__":
|
||||
try:
|
||||
sys.exit(main())
|
||||
except Mismatch as e:
|
||||
print("\nVERDICT: MISMATCH -- %s" % e)
|
||||
sys.exit(1)
|
||||
Reference in new issue
Block a user