refactor(cut-criteria): unify 3 bash scripts into one Python module#395
Closed
trilamsr wants to merge 1 commit into
Closed
refactor(cut-criteria): unify 3 bash scripts into one Python module#395trilamsr wants to merge 1 commit into
trilamsr wants to merge 1 commit into
Conversation
Replace `scripts/cut-criteria-{status,render,status_test}.sh` with a
single `scripts/cut_criteria.py` exposing `status`, `render`, `check`,
and `test` subcommands. Output is byte-identical for the same YAML +
repo state (the only deliberate diff is the script-name reference line
in the rendered legend).
Drops:
- The base64-over-TSV plumbing in cut-criteria-status.sh. It existed
only to survive embedded newlines through a bash `read` loop, a
macOS-3.2 compat tax. subprocess.run(snippet, shell=True) passes
multi-line YAML block scalars to /bin/bash directly, no encoding
hop. A regression fixture (`multiline-shell-no-base64`) locks this
in: a future refactor that reintroduces a TSV/base64 round-trip
without honoring newlines goes red.
- The per-shell-invocation Python heredocs in render.sh (196 LOC) and
status.sh (75 LOC). Three Makefile shell-outs to one module now
replace three bash scripts that each shelled out to python3.
- The separate `*_test.sh` test harness (160 LOC). The fixture suite
is now a `cut_criteria.py test` subcommand using the same in-process
status_rows() the production path uses.
Adds (per issue ask):
- YAML schema validation at parse time (required keys, allowed tiers,
duplicate-id check) — surfaces malformed spec at `make
cut-criteria-status` instead of producing a quietly-wrong markdown.
- `check` subcommand that renders in-process and diffs against the
on-disk markdown; no `mktemp`/`diff -u`/`rm -f` choreography. The
Makefile target collapsed from 11 lines to 1.
Closes #386.
Signed-off-by: Tri Lam <tri@maydow.com>
Contributor
Author
|
Superseded by #397 (same commits, properly-named branch). |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Replace
scripts/cut-criteria-{status,render,status_test}.shwith a singlescripts/cut_criteria.pymodule exposingstatus,render,check, andtestsubcommands. Output is byte-identical for the same YAML + repo state — the only deliberate diff in the rendered markdown is the script-name reference line in the legend.Closes #386.
Why
PR #374 wrapped each criterion's
rubric_checkshell snippet in base64 to ship it through a TSV row format the bashwhile read -d ''loop could parse. That hop existed only because bash's row parser cannot survive literal newlines inside fields. Root cause: shell-as-row-parser against YAML block scalars. The reviewer + the original author both flagged it as over-engineered.Root cause + fix
base64 -dper row\ninside fieldssubprocess.run(snippet, shell=True, executable="/bin/bash")passes multi-line block scalars to bash directly.*_test.sh(160 LOC) of bash assertionscut_criteria.py testreusesstatus_rows()against in-process fixtures.cut-criteria-checkMakefile target (mktemp+diff -u+rm -f)difflib; Makefile target is now 1 line.What landed
scripts/cut_criteria.py— single module. CLI:status | render | check | test.make cut-criteria-statusinstead of producing a quietly-wrong markdown.multiline-shell-no-base64— locks in the refactor; a future change that reintroduces a TSV/base64 hop without honoring newlines goes red.scripts/cut-criteria-status.sh,scripts/cut-criteria-render.sh,scripts/cut-criteria-status_test.sh.docs/cut-criteria.yaml{,.md}updated to point at the new entrypoints.TDD evidence
status.tsv+ renderedv1-rc1-cut-criteria.mdBEFORE editing (against the live repo state).Test plan
python3 scripts/cut_criteria.py test— 7/7 fixtures pass (6 carried forward from the bash suite + 1 new multi-line regression).make cut-criteria-status— 15-row output matches the pre-refactor golden byte-for-byte.make cut-criteria-render— idempotent (two consecutive renders → no diff on disk).make cut-criteria-check— clean on this branch.make doc-check— clean (transitively runscut-criteria-check).make actionlint— clean.python3 -m py_compile scripts/cut_criteria.py— clean.Out of scope (deliberate)
git status -s docs/v1-rc1-cut-criteria.mdas a candidate drift gate. Kept the in-process diff:git status -sonly catches uncommitted local edits, not a deterministically-stale-in-the-repo render (CI's actual failure mode). The in-process diff is strictly simpler than the bash temp-file diff it replaces (1-line Makefile target vs 11 lines).