|
| 1 | +--- |
| 2 | +name: post-change-refactor |
| 3 | +description: >- |
| 4 | + Refactor and clean up the uncommitted change before committing. Eliminate |
| 5 | + duplication, rename unclear domain concepts, collapse shotgun surgery when |
| 6 | + another similar change is likely, remove dead / test-only / redundant code, |
| 7 | + split files larger than 250 lines into cohesive modules, and confirm |
| 8 | + related tests still pass. Use after finishing a phase or sub-phase and |
| 9 | + before commit, or whenever the developer asks to clean up the current |
| 10 | + change. Triggers on: refactor change, clean up change, post-change |
| 11 | + refactor, before commit cleanup, tidy current change. |
| 12 | +--- |
| 13 | + |
| 14 | +# Post-Change Refactor — Clean the Current Change Before Commit |
| 15 | + |
| 16 | +## When to use |
| 17 | + |
| 18 | +- After the implementation of a phase / sub-phase finishes and **before** |
| 19 | + the commit. |
| 20 | +- On demand, when the developer asks to clean up or refactor the current |
| 21 | + uncommitted change. |
| 22 | + |
| 23 | +Start with `.cursor/rules/` for project conventions and test practices. |
| 24 | + |
| 25 | +## Scope: "the current change" |
| 26 | + |
| 27 | +The current change = files touched since the last commit (staged + unstaged |
| 28 | ++ untracked). Discover the scope: |
| 29 | + |
| 30 | +```bash |
| 31 | +git status |
| 32 | +git diff |
| 33 | +git diff --cached |
| 34 | +``` |
| 35 | + |
| 36 | +**Git does not use the Nix prefix.** All other repo tooling does. |
| 37 | + |
| 38 | +All refactoring work is scoped to those files and the files that |
| 39 | +directly depend on them or are depended on by them. Do **not** sweep |
| 40 | +unrelated parts of the repo. |
| 41 | + |
| 42 | +## Decision boundary |
| 43 | + |
| 44 | +Only keep code that is justified by **the current change** or by the |
| 45 | +**immediate next phase** in the plan (if one exists). |
| 46 | +Anything justified only by a phase further out, or by "we might need it |
| 47 | +later", is speculative — remove it. |
| 48 | + |
| 49 | +If there is no plan, justification must come entirely from the current |
| 50 | +change. |
| 51 | + |
| 52 | +## Procedure |
| 53 | + |
| 54 | +Run each check below in order. After all checks pass, **hand control back |
| 55 | +to the caller** — do **not** commit from inside this skill. |
| 56 | + |
| 57 | +### 1. Duplication |
| 58 | + |
| 59 | +- Look for **new** duplication introduced by the change (copy-pasted |
| 60 | + blocks, parallel structures with cosmetic differences). |
| 61 | +- Look for duplication the change made **visible** — the new code repeats |
| 62 | + logic that already existed elsewhere. |
| 63 | +- The same concept appearing in two representations counts as duplication, |
| 64 | + not just literal copies. |
| 65 | +- **Action**: collapse onto a single representation. Prefer reusing an |
| 66 | + existing helper in the right layer (provider, `Translator`, CLI) over |
| 67 | + inventing a new one. |
| 68 | + |
| 69 | +### 2. Domain naming |
| 70 | + |
| 71 | +- Read every new or renamed identifier — files, modules, classes, |
| 72 | + functions, variables, tests, fixtures. |
| 73 | +- Ask: does the name match what a domain reader expects? Does it match |
| 74 | + translate-python's language (providers, translation, CLI, languages, |
| 75 | + API keys, etc.)? |
| 76 | +- **Action**: rename when intent is unclear, misleading, mixes layers, or |
| 77 | + leaks phase numbers / sequence info. Names describe **capability**, not |
| 78 | + development history. |
| 79 | + |
| 80 | +### 3. Shotgun surgery |
| 81 | + |
| 82 | +- A change is shotgun surgery when **one logical concept** forces edits |
| 83 | + in many places. |
| 84 | +- Estimate the likelihood of another change of the same shape in the |
| 85 | + foreseeable future. |
| 86 | +- **Action**: |
| 87 | + - **High likelihood** → consolidate the scattered edits behind a single |
| 88 | + seam (one function, one config, one module) so the next change touches |
| 89 | + one place. Do it now. |
| 90 | + - **Low likelihood** → leave it. Do not preemptively abstract. |
| 91 | + |
| 92 | +### 4. Dead / redundant / cancelling code |
| 93 | + |
| 94 | +Remove aggressively whatever the change introduced or exposed that is not |
| 95 | +justified by the current change or the immediate next phase: |
| 96 | + |
| 97 | +- Code with no caller. |
| 98 | +- Branches that cannot be reached. |
| 99 | +- Pairs of edits that cancel each other (added then immediately worked |
| 100 | + around, flag toggles that never flip, etc.). |
| 101 | +- Production code only exercised by unit tests — no real caller from the |
| 102 | + CLI (`translate-cli`), `Translator` API, or provider pipeline. |
| 103 | +- Unit tests that overlap with another test on the same observable |
| 104 | + surface (same input/output, same entry point). |
| 105 | +- Tests that pin internal structure rather than observable behavior — if |
| 106 | + a test mainly mirrors the code's factoring, drop it in favor of the |
| 107 | + test that drives a high-level entry point (`translate-cli`, `Translator.translate`, |
| 108 | + CLI output). |
| 109 | + |
| 110 | +When in doubt, **delete**. The next phase, if relevant, will reintroduce |
| 111 | +only what it actually needs. |
| 112 | + |
| 113 | +### 5. File size |
| 114 | + |
| 115 | +For every file touched by the change, check line count: |
| 116 | + |
| 117 | +```bash |
| 118 | +wc -l <path> |
| 119 | +``` |
| 120 | + |
| 121 | +- Files **over 250 lines** must be split. This rule is applied to test code as well. |
| 122 | +- Split along **cohesive seams** — one concept per module, not arbitrary |
| 123 | + line cuts. |
| 124 | +- Update imports. Keep the public API stable for callers outside the |
| 125 | + change. |
| 126 | + |
| 127 | +### 6. Confirm related tests still pass |
| 128 | + |
| 129 | +Run **related** tests for the changed files — not the whole suite. Use |
| 130 | +`nix develop -c …` for all commands below except `git`. |
| 131 | + |
| 132 | +| Area touched | Focused command | |
| 133 | +|--------------|-----------------| |
| 134 | +| Core translation (`translate/translate.py`) | `nix develop -c pytest tests/test_translate.py` | |
| 135 | +| Provider (`translate/providers/`) | `nix develop -c pytest tests/test_provider.py` | |
| 136 | +| CLI / options (`translate/main.py`, `translate/__main__.py`) | `nix develop -c pytest tests/test_main.py` | |
| 137 | +| Broad or cross-cutting change | `nix develop -c pytest` | |
| 138 | + |
| 139 | +Prefer CLI integration tests and VCR-backed translation tests over tests |
| 140 | +that only exercise internal helpers. |
| 141 | + |
| 142 | +All related tests must pass before returning. If a test breaks because of |
| 143 | +the refactor (not the original change), fix it now. |
| 144 | + |
| 145 | +## Return |
| 146 | + |
| 147 | +Report a short summary to the caller: |
| 148 | + |
| 149 | +1. Which checks led to changes — duplication / naming / shotgun / |
| 150 | + dead code / file size. |
| 151 | +2. Files renamed, extracted, split, or deleted. |
| 152 | +3. Which related tests were run and confirmed passing. |
| 153 | + |
| 154 | +Then hand control back. **Do not commit from inside this skill** — the |
| 155 | +caller commits. |
| 156 | + |
| 157 | +## Out of scope |
| 158 | + |
| 159 | +- Do not redesign code outside the changed files. |
| 160 | +- Do not start a new phase or add new behavior. Only restructure and |
| 161 | + remove. |
| 162 | +- Do not run the entire test suite or trigger CI from inside this skill |
| 163 | + unless the change is broad enough that focused tests cannot cover it. |
0 commit comments