|
| 1 | +# Plan: Decouple inferType from Raw Positional Arguments (#199) |
| 2 | + |
| 3 | +## Reference spec |
| 4 | +[docs/specs/2026-05-18-199-infer-type-context-object.md](../specs/2026-05-18-199-infer-type-context-object.md) |
| 5 | + |
| 6 | +## Approach |
| 7 | + |
| 8 | +Introduce `InferTypeContext` in `src/journal/paths.ts` (colocated with `inferType` — approved in spec review). Change the function signature from `(entry, extension: string)` to `(entry, ctx: InferTypeContext)`. Update both call sites in `scan-entries.ts`. No logic changes — pure structural refactor. |
| 9 | + |
| 10 | +Trade-off: keeping the interface in `paths.ts` rather than `src/model/interfaces.ts` maximises local cohesion at the cost of discoverability. Correct per YAGNI until a second consumer appears. |
| 11 | + |
| 12 | +**Amendment (post-review):** Unit tests must be written against existing behavior before touching the signature, to lock down the contract empirically. All future fields in `InferTypeContext` must be optional (`?:`) — required fields would reintroduce shotgun surgery on every addition. The `|` literal in the existing regex (`/^[\d|\-|_]+$/`) is a pre-existing quirk (pipes illegal in Windows filenames, unused on Unix); it is documented in the test but not fixed in this PR. |
| 13 | + |
| 14 | +## Steps |
| 15 | + |
| 16 | +### 0 — Write unit tests for current `inferType` behavior (`src/test/suite/infer-type.test.ts`) |
| 17 | + |
| 18 | +`inferType` has no VS Code dependencies — safe to run inside Extension Host suite without special plumbing. Lock down all three classification branches before touching the signature: |
| 19 | + |
| 20 | +- attachment: extension mismatch → `JournalPageType.attachment` |
| 21 | +- entry: `extension` matches AND name matches `/^[\d|\-|_]+$/` → `JournalPageType.entry` |
| 22 | +- note: `extension` matches AND name is alphanumeric → `JournalPageType.note` |
| 23 | + |
| 24 | +Include a test that confirms the current (pre-fix) behavior of `2026|05|18.md` → `entry`, so the regex fix in Step 0b is verifiable. |
| 25 | + |
| 26 | +### 0b — Fix regex in `inferType` (`src/journal/paths.ts`) |
| 27 | + |
| 28 | +Separate commit. Change `/^[\d|\-|_]+$/gm` → `/^[\d\-_]+$/`: |
| 29 | + |
| 30 | +- Remove `|` literal from character class (was unintentionally included; pipe is valid on macOS/Linux) |
| 31 | +- Remove `gm` flags (unnecessary for a single filename string match) |
| 32 | + |
| 33 | +No classification semantics change for real-world filenames. Update the test from Step 0 to reflect corrected behavior: `2026|05|18.md` → `JournalPageType.note` after fix. |
| 34 | + |
| 35 | +### 1 — Add `InferTypeContext` and update `inferType` (`src/journal/paths.ts`) |
| 36 | + |
| 37 | +Define the interface immediately above the function: |
| 38 | + |
| 39 | +```typescript |
| 40 | +export interface InferTypeContext { |
| 41 | + extension: string; |
| 42 | + // all future fields must be optional (?: ) to prevent shotgun surgery |
| 43 | +} |
| 44 | +``` |
| 45 | + |
| 46 | +Change signature: |
| 47 | + |
| 48 | +```typescript |
| 49 | +export function inferType(entry: Path.ParsedPath, ctx: InferTypeContext): J.Model.JournalPageType |
| 50 | +``` |
| 51 | + |
| 52 | +Replace `extension` references inside the body with `ctx.extension`. |
| 53 | + |
| 54 | +### 2 — Update call sites (`src/features/entries/scan-entries.ts`) |
| 55 | + |
| 56 | +Two occurrences (lines 68 and 140 at time of writing): |
| 57 | + |
| 58 | +```typescript |
| 59 | +// before |
| 60 | +entry.type = J.Journal.inferType(Path.parse(entry.path), this.config.getFileExtension()); |
| 61 | +// after |
| 62 | +entry.type = J.Journal.inferType(Path.parse(entry.path), { extension: this.config.getFileExtension() }); |
| 63 | +``` |
| 64 | + |
| 65 | +### 3 — Verify compile |
| 66 | + |
| 67 | +```bash |
| 68 | +npm run compile # esbuild — extension bundle |
| 69 | +npm run compile-tests # tsc — catches type errors in tests |
| 70 | +``` |
| 71 | + |
| 72 | +### 4 — Run tests |
| 73 | + |
| 74 | +```bash |
| 75 | +npm test |
| 76 | +``` |
| 77 | + |
| 78 | +All existing tests plus the new unit tests must pass. |
| 79 | + |
| 80 | +## Test scenarios |
| 81 | + |
| 82 | +- **Compile clean:** `npm run compile` and `npm run compile-tests` both exit 0, no TS errors |
| 83 | +- **Regression — attachment:** file with non-matching extension → `JournalPageType.attachment` |
| 84 | +- **Regression — entry:** matching extension + digits/dashes/underscores name → `JournalPageType.entry` |
| 85 | +- **Regression — note:** matching extension + alphanumeric name → `JournalPageType.note` |
| 86 | +- **Regex fix verified:** `2026|05|18.md` → `JournalPageType.note` after Step 0b (pipe no longer in character class) |
| 87 | +- **Extensibility proof:** adding `weeklyFilePattern?: string` to `InferTypeContext` requires touching only `paths.ts` |
| 88 | + |
| 89 | +## Dependencies |
| 90 | + |
| 91 | +None. Self-contained structural change; no other open PR touches `inferType` or its call sites. |
| 92 | + |
| 93 | +## Risk |
| 94 | + |
| 95 | +Low. Pure signature refactor — no logic change, no new code paths. Covered entirely by the existing test suite plus compile checks. |
| 96 | + |
| 97 | +## Rollback |
| 98 | + |
| 99 | +`git revert <commit>` — single commit, no data or config side effects. |
0 commit comments