|
| 1 | +# Issue #177 — Replace smart-input regex with a proper parser |
| 2 | + |
| 3 | +> **Issue:** [pajoma/vscode-journal#177](https://github.com/pajoma/vscode-journal/issues/177) |
| 4 | +> **Created:** 2026-06-01 |
| 5 | +
|
| 6 | +## Goal |
| 7 | + |
| 8 | +Replace the single assembled regex in `src/journal/match-input.ts` (`getExpression()`) with a structured tokenizer + locale-vocab-table approach that eliminates alternation-order fragility, enables exhaustive testing, and makes locale extension declarative. |
| 9 | + |
| 10 | +## Why now |
| 11 | + |
| 12 | +Issue #170 patched the immediate prefix-collision bug with `(?=\s|$)` lookaheads inside the regex. That fix closes the symptom for known cases but does not remove the structural root cause: alternation order in a single opaque regex is invisible, brittle, and impossible to cover exhaustively. Every new locale, shortcut, or syntax form (#230 adds week-boundary shortcuts and 2-letter aliases) requires re-deriving the whole regex's behaviour by hand. The 1.2.0 roadmap treats this as the strategic prerequisite for any further smart-input work. |
| 13 | + |
| 14 | +## Current structure (what we are replacing) |
| 15 | + |
| 16 | +`src/journal/match-input.ts` — 502 lines, one class: |
| 17 | + |
| 18 | +- `getExpression()` — assembles a single named-group regex from component string fragments. Cached in `this.expr`. |
| 19 | +- `getWeekdayPattern()` — builds a locale-keyed alternation string (`monday|...|montag|...|lun(?:di)?|...`) joined from arrays. Already structured as arrays; currently joined into a regex alternation. |
| 20 | +- `getMonthPattern()` — same shape for month names across 10 locales. |
| 21 | +- `parseInput()` — runs the regex, then post-processes named capture groups into an `Input` object. Most branching lives here (~200 lines of `if/switch`). |
| 22 | + |
| 23 | +The `Input` model (`src/model/`) is **unchanged** by this issue — it is the stable public contract. |
| 24 | + |
| 25 | +## Option analysis |
| 26 | + |
| 27 | +### Option A — Hand-written tokenizer (sequential recognizers) |
| 28 | + |
| 29 | +Replace `getExpression()` with a `Tokenizer` that walks the input string left-to-right, trying each recognizer in priority order. Each recognizer claims a prefix, returns a typed `Token`, and advances the cursor. `parseInput()` then maps the token stream to an `Input`. |
| 30 | + |
| 31 | +**Bundle size:** zero added deps. Comparable code size to current 502-line file. |
| 32 | +**Readability:** each recognizer is a small, named function — individually readable and unit-testable. |
| 33 | +**Locale extensibility:** locale vocab stays in the existing arrays; recognizers reference those arrays directly. Adding a locale = add to the vocab array only. |
| 34 | +**Test coverage:** each recognizer can be unit-tested in isolation (pure functions, no regex). Property-based tests (e.g. fuzz over all weekday abbreviations) are straightforward. |
| 35 | +**Breaking-change risk:** zero — `Input` model unchanged; `parseInput()` signature unchanged. |
| 36 | +**Trade-off:** initial rewrite effort; must replicate all edge cases (ISO date formats, week-number variants, offset `+N`/`-N`, `task`/`todo` flags pre/post). Requires a careful migration phase to run old and new in parallel until parity is confirmed. |
| 37 | + |
| 38 | +### Option B — Parser combinator (arcsecond or tiny in-tree) |
| 39 | + |
| 40 | +A parser-combinator library lets you write grammar rules as composable functions (`seq`, `alt`, `many`, `map`). Either pull in `arcsecond` (~15 kB minified) or write a 100-line in-tree micro-combinator. |
| 41 | + |
| 42 | +**Bundle size:** `arcsecond` is not tree-shaken by esbuild in all cases; ~8 kB net after shake. In-tree combinator adds ~100 lines. |
| 43 | +**Readability:** grammar is declarative and compositional. Very readable once familiar with the idiom. |
| 44 | +**Locale extensibility:** same as Option A — vocab tables drive the leaf parsers. |
| 45 | +**Test coverage:** combinators are composable and individually testable. |
| 46 | +**Breaking-change risk:** zero for `Input` model; low for bundle if in-tree. |
| 47 | +**Trade-off:** external dep (or non-trivial in-tree code) for a domain that is simpler than what combinators are designed for. Combinator error messages are harder to surface in VS Code toasts. |
| 48 | + |
| 49 | +### Option C — Lookup-driven dispatch (vocab tables + priority scan) |
| 50 | + |
| 51 | +`getWeekdayPattern()` and `getMonthPattern()` already produce sorted arrays per locale. Option C keeps those tables, stops joining them into regex strings, and drives a scan-loop that tries each vocab entry as a `startsWith` (case-insensitive, word-boundary checked). The overall parse sequence mirrors Option A but the recognizers are data-driven rather than function-based. |
| 52 | + |
| 53 | +**Bundle size:** zero deps. |
| 54 | +**Readability:** vocab tables are easy to read; scan logic is ~30 lines. |
| 55 | +**Locale extensibility:** best of the three — locale data is entirely declarative. |
| 56 | +**Test coverage:** scan loop is a pure function over a vocab table, trivially testable. |
| 57 | +**Breaking-change risk:** zero. |
| 58 | +**Trade-off:** less composable than combinator; the scan loop must enforce priority order explicitly (longest-match-first), which requires sorting vocab entries by descending length before scanning. |
| 59 | + |
| 60 | +## Recommendation |
| 61 | + |
| 62 | +**Option A (tokenizer) with Option C's vocab-table data layer.** |
| 63 | + |
| 64 | +Concretely: |
| 65 | +1. Keep `getWeekdayPattern()` and `getMonthPattern()` as array-returning functions (remove the final `.join('|')` and regex wrapping). Name them `WEEKDAY_VOCAB` and `MONTH_VOCAB`. |
| 66 | +2. Implement a `Tokenizer` class (or module-level function set) with one recognizer per token type: `recognizeFlag`, `recognizeShortcut`, `recognizeOffset`, `recognizeISO`, `recognizeWeekNum`, `recognizeWeekBoundary`, `recognizeWeekday`, `recognizeMonth`, `recognizeText`. |
| 67 | +3. Each recognizer is a pure function `(input: string, pos: number, locale: string) => Token | null`. Longest-match-first for locale vocab; word-boundary enforced via a helper (replicates the `(?=\s|$)` fix from #170). |
| 68 | +4. `parseInput()` calls `tokenize(input, locale)` → `Token[]`, then maps to `Input` (the existing post-processing logic survives largely intact, just reading token types instead of named groups). |
| 69 | +5. The old regex path is deleted once the new tokenizer passes all existing tests plus the #170 regression suite. |
| 70 | + |
| 71 | +This approach has zero external dependencies, keeps locale data declarative, makes every token type independently unit-testable, and mirrors the existing code structure closely enough to minimize migration risk. |
| 72 | + |
| 73 | +## Acceptance criteria |
| 74 | + |
| 75 | +### Design phase (this spec) |
| 76 | + |
| 77 | +- Trade-off analysis covering all three options with bundle size, readability, locale extensibility, test coverage, and breaking-change risk — **done above**. |
| 78 | +- Recommendation with rationale — **done above**. |
| 79 | +- Migration plan that preserves the published 1.x `Input` contract — see section below. |
| 80 | +- Success metrics defined — see section below. |
| 81 | + |
| 82 | +### Implementation phase (to be detailed in the plan) |
| 83 | + |
| 84 | +- All 34 existing test cases in `input.test.ts` and `week-input.test.ts` pass with the new tokenizer. |
| 85 | +- The #170 regression cases (German `do`/`di`/`fr`, Dutch `do`/`vr`, Spanish `mar`/`vie` prefix collisions) pass. |
| 86 | +- A new property-based test suite (`src/test/suite/match-input-vocab.test.ts`) sweeps all weekday and month abbreviations across all supported locales and asserts no false-positive match against a corpus of common everyday words that share prefixes. |
| 87 | +- `npm run check` (lint + compile + full test) green on CI. |
| 88 | +- No change to the exported `Input` type or the `parseInput(inputString: string): Promise<Input>` signature. |
| 89 | + |
| 90 | +## Migration plan |
| 91 | + |
| 92 | +1. **Parallel phase:** Add `tokenize()` alongside the existing `getExpression()`. Run both paths in `parseInput()`; log a warning when outputs differ. Use this to surface edge cases in the test workspace before removing the regex. |
| 93 | +2. **Parity phase:** Add test cases for every discovered divergence until both paths agree. Extend the property-based test suite. |
| 94 | +3. **Cutover:** Remove `getExpression()`, the `this.expr` cache, and the parallel-run scaffolding. `parseInput()` uses tokenizer only. |
| 95 | + |
| 96 | +No feature-flag needed — the parallel phase is purely internal to `parseInput()`, invisible to callers. |
| 97 | + |
| 98 | +## Success metrics |
| 99 | + |
| 100 | +| Metric | Target | |
| 101 | +|--------|--------| |
| 102 | +| Existing `input.test.ts` cases | All 30 pass | |
| 103 | +| Existing `week-input.test.ts` cases | All 4 pass | |
| 104 | +| #170 prefix-collision regression cases | All pass | |
| 105 | +| New vocab property-based sweep | Zero false-positive weekday/month matches | |
| 106 | +| `npm run check` | Green | |
| 107 | +| Bundle size delta | < +2 kB minified (zero external deps) | |
| 108 | + |
| 109 | +## Constraints |
| 110 | + |
| 111 | +- No change to `Input` model or `parseInput()` public signature. |
| 112 | +- No new external runtime dependencies (only devDependencies for test tooling if needed). |
| 113 | +- Must work in both local and Remote SSH/Codespaces extension hosts (pure TS logic, no Node built-ins beyond what already exists). |
| 114 | +- Target: `src/journal/match-input.ts` remains the single home for parsing logic. |
| 115 | + |
| 116 | +## Out of scope |
| 117 | + |
| 118 | +- QuickPick/InputBox surface changes. |
| 119 | +- Localization of UI strings (handled by `vscode.l10n`, issue #176). |
| 120 | +- Moment.js removal (PLAN.md Phase 3). |
| 121 | +- Adding new syntax forms (tracked separately in #230 — but the new tokenizer must be extensible enough to absorb those changes without structural surgery). |
| 122 | + |
| 123 | +## Related |
| 124 | + |
| 125 | +- **Blocked by:** none (PR for #170 already merged or in flight; this issue follows). |
| 126 | +- **Related:** #170 (prefix-collision fix that motivated this), #167 (regex/data drift bug family), #230 (week-boundary + 2-letter alias — new syntax that will land on the new tokenizer). |
| 127 | +- **PLAN.md:** no current phase explicitly tracks this; 1.2.0 milestone. |
0 commit comments