refactor: delete three unreferenced functions - #50
Conversation
`knip` over the whole project — with `src/cli.ts`, `src/templates.ts`, `scripts` and the test suites as entry points — reports 68 exports with no consumer outside their own file. Only three of them are unreferenced anywhere: - `sdkComponentName` in `check-android-sdk.ts`, a one-line wrapper around `basename` - `executableDirectory` in `doctor-environment.ts`, a one-line `extname` ternary - `planOwnedForwards` in `apply-forwards.ts`, which pre-computed the ownership a detached localnet session was about to claim The third is the only substantive one, and it is obsolete rather than unfinished: the detach path records ownership from the forwards that actually landed (`localnet-feature-start.ts`, `writeOwned(mergeOwnedForwards(...))` over `owned.values()`) instead of from a pre-applied plan, and the comment there explains why that ordering is the safe one. Its own doc comment described the superseded approach. The other 65 findings are left alone deliberately. Every one is referenced inside its own file — they are dependency-injection interfaces (`RunLocalnetStartDependencies`, `EnsureKeystoreDependencies` and friends), the `default*` injection helpers they carry, and module constants. Dropping `export` from them would touch some 25 files, remove the documented injectable surface this repo exports consistently, and change nothing that runs. `AppOptions` is in that set too and stays exported: it is the argument contract for `createApp` and `runApp`. knip is not added as a dependency. With 65 of 68 findings intentional, a gate would need 65 ignores to stay green and would report nothing useful; it is more valuable run by hand when the question comes up again.
|
commit: |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (3)
Included review availability: 1 review is currently available. Your included PR review attempts over the past 7 days set your current allowance at 2 reviews per hour. 📝 WalkthroughWalkthroughThe changes remove unused imports and three exported helper functions from doctor and localnet data-access modules. ChangesUnused helper and import cleanup
Estimated code review effort: 1 (Trivial) | ~3 minutes Merge Risk: ⚪ Minimal · up to This cleanup removes obsolete helper APIs without changing active behavior. Current evidence indicates no merge-blocking risk. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
1 participant
Last item from the cleanup audit that followed the command-factory series. It is much smaller than expected, and that is the main finding.
What the audit claim was, and what is actually true
A grep-based scan suggested "~72 exports with no consumer" and I called it a mechanical pass. Running
knipover the whole project — entry pointssrc/cli.ts,src/templates.ts,scripts/*.tsand every test suite — and then classifying each hit by whether the symbol is referenced inside its own file:default*injection helpers, module constantsSo this PR deletes three functions. It is not a sweep.
The three
sdkComponentName(check-android-sdk.ts) — one-line wrapper aroundbasenameexecutableDirectory(doctor-environment.ts) — one-lineextnameternaryplanOwnedForwards(apply-forwards.ts) — 18 lines that pre-computed the ownership a detached localnet session was about to claimThe third was worth checking rather than trusting the tool on, because its doc comment described real behaviour and dead code sometimes means an unfinished feature. It is obsolete, not missing: the detach path records ownership from the forwards that actually landed —
writeOwned(mergeOwnedForwards(...))overowned.values()inlocalnet-feature-start.ts— rather than from a pre-applied plan, and the comment there explains why that ordering is the safe one.planOwnedForwards' own comment described the approach that replaced it.Three imports narrowed as a result:
basename,dirname/extname/resolve, andownedForwards.The 65 are left alone deliberately
Every one is referenced inside its own file. They are dependency-injection interfaces (
RunLocalnetStartDependencies,EnsureKeystoreDependencies,ReadWebshellManifestDependenciesand friends), thedefault*helpers they carry, and module constants. Droppingexportfrom them would touch some 25 files, delete the documented injectable surface this repo exports consistently, and change nothing that runs.AppOptionsis in that set and stays exported: it is the argument contract forcreateAppandrunApp.knip is not added as a dependency
With 65 of 68 findings intentional, a CI gate would need 65 ignores to stay green and would report nothing useful. It is more valuable run by hand when the question comes up again. Wiring it in permanently with a deliberate ignore list is a reasonable separate change if you want the standing check.
Verification
bun run ci— build, lint,tsc -b --noEmit, 487 pass / 0 fail. A knip re-run confirms 68 → 65 with exactly these three gone.Noted, not addressed
Two duplications surfaced along the way, both behavioural questions rather than moves:
doctor/data-access/check-android-devices.tshas its ownparseAdbDevicesandAdbDevice, unrelated to the ones indevice/data-accessand carrying akindfielddoctor/data-access/doctor-environment.tshas its ownrunExecutable, distinct fromcore/data-access/run-executable.tsand with a different signature —(command, args)returning{ path, stdout, stderr }versus(cmd[])returning a stringDoctor has quietly grown a parallel command layer. Consolidating it would change what doctor reports, so it does not belong in a dead-code PR.
Summary by CodeRabbit