Add Vale prose linting#50
Conversation
PR Validation ❌Some checks failed. Please review the errors above. Checks Performed:Please check the workflow logs for details on which checks failed. This is a complete dry-run of the deployment process, ensuring your changes will deploy successfully when merged. |
This comment has been minimized.
This comment has been minimized.
|
Addressed in c934390.
Verified with |
PR Validation ❌Some checks failed. Please review the errors above. Checks Performed:Please check the workflow logs for details on which checks failed. This is a complete dry-run of the deployment process, ensuring your changes will deploy successfully when merged. |
Review: Add Vale prose lintingNice addition overall — the Vale wiring is clean, consistent with the existing spellcheck/lint conventions (npm scripts → justfile → CI reusable workflow → docs), and Test coverage
Minor / non-blocking
Things that look right
Nothing here blocks merging — the test-coverage gap is the one item I'd actually act on before or shortly after merge. |
PR Validation ✅All checks passed! Ready for review. Checks Performed:✓ Linting This is a complete dry-run of the deployment process, ensuring your changes will deploy successfully when merged. |
Review: Add Vale prose lintingNice addition — wiring Vale into 1. Fixture tests don't cover half the substitution rules
Not exercised by 2. Overlapping rules can double-flag the same text The Minor / non-blocking
No security or performance concerns — this only touches devDependencies/CI tooling and doesn't affect the built site's runtime. |
PR Validation ✅All checks passed! Ready for review. Checks Performed:✓ Linting This is a complete dry-run of the deployment process, ensuring your changes will deploy successfully when merged. |
Review: Add Vale prose lintingNicely scoped addition — the CI wiring, docs, justfile, and fixture-based regression tests for the Terminology style all fit together cleanly, and the follow-up commits already addressed the overlap/duplicate-alert and iPad/iPadOS-scoping issues from earlier review passes. A few remaining notes: Correctness (minor)
'\b(ipad(?![ -]?(?:os|OS)\b)|IPad(?![ -]?OS\b)|Ipad(?![ -]?OS\b)|IPAD)\b': iPadThe plain Security (informational, not blocking)
Test coverage
Everything else
Overall this looks ready to merge; the iPad/iPadOS lookahead inconsistency is the only thing I'd consider actually fixing, and even that's low severity. |
PR Validation ✅All checks passed! Ready for review. Checks Performed:✓ Linting This is a complete dry-run of the deployment process, ensuring your changes will deploy successfully when merged. |
Review: Add Vale prose lintingNicely scoped addition — the Vale wiring, CI parity docs, justfile targets, and fixture-based regression test are all consistent with each other, and Correctness:
|
PR Validation ✅All checks passed! Ready for review. Checks Performed:✓ Linting This is a complete dry-run of the deployment process, ensuring your changes will deploy successfully when merged. |
|
Review: Add Vale prose linting Nice addition overall the fixture driven test approach in scripts/check-vale-fixtures.js, asserting exact match sets rather than just pass or fail, is a good pattern; it will catch missing rules and duplicate or overlapping alerts, which the commit history shows it already did during review iteration. Docs (CI_CD_DOCUMENTATION.md, WORKFLOW_ARCHITECTURE.md, VALIDATION.md, .vale/README.md, CLAUDE.md, justfile) are updated consistently, and CI wiring (build.yml, pr-validation.yml) looks correct new steps run after npm ci (so the Vale binary from @vvago/vale postinstall is present) and before the build. Code quality / maintainability
Potential bugs
Performance Security
Test coverage
Nothing here blocks merging the core wiring is correct and well-tested; the notes above are mostly about long-term maintainability of the regex rules as more Apple terms get added. |
PR Validation ✅All checks passed! Ready for review. Checks Performed:✓ Linting This is a complete dry-run of the deployment process, ensuring your changes will deploy successfully when merged. |
Review: Add Vale prose lintingWent through the diff (config, scripts, styles, fixtures, CI/docs wiring) and traced the What's good
Minor / non-blocking notes
No functional bugs found in the regex logic, script error handling, or CI wiring. Test coverage (valid + invalid fixtures, exact-match assertions) is appropriately rigorous for a linting rule that's proven fiddly to get right across the commit history. |
Adds project-local Vale configuration and a starter Terminology style for article Markdown/MDX, focused on Apple capitalization consistency.
Wires
lint:prosethrough npm scripts, the justfile, local CI parity, and the reusable GitHub Actions validation workflow.Updates validation docs and fixes an existing Objective-C capitalization issue in content.
Review follow-up:
Validation:
npm ci,npm run lint:prose,just lint-prose,npm run test:ci,npm run lint:markdown,git diff --check.Latest local validation:
npm run test:ci.