refactor(stringutil): extract isASCIIAlphanumeric helper in sanitize.go#47455
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Extracts a shared ASCII alphanumeric check to reduce duplicated sanitization logic without changing behavior.
Changes:
- Adds
isASCIIAlphanumeric. - Reuses it in both identifier and filename sanitizers.
Show a summary per file
| File | Description |
|---|---|
pkg/stringutil/sanitize.go |
Consolidates duplicate character validation logic. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 1/1 changed files
- Comments generated: 0
- Review effort level: Medium
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #47455 does not have the 'implementation' label and has only 7 new lines of code in business logic directories (threshold: 100). |
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
✅ Test Quality Sentinel completed test quality analysis. No test files were added or modified in this PR. Test Quality Sentinel skipped. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
There was a problem hiding this comment.
Clean refactor — the extracted isASCIIAlphanumeric helper correctly mirrors the original inline conditions and is applied consistently at both call sites. No logic changes, no issues found.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 8.87 AIC · ⌖ 5.1 AIC · ⊞ 5K
There was a problem hiding this comment.
Clean refactor — approved
Extracting isASCIIAlphanumeric is a correct, behavior-preserving deduplication. Both call sites updated properly, helper is appropriately unexported, and existing tests cover both paths. No issues found.
🔎 Code quality review by PR Code Quality Reviewer · sonnet46 17.9 AIC · ⌖ 4.47 AIC · ⊞ 5.7K
Comment /review to run again
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /codebase-design — the extraction is clean and behavior-preserving. ✅
📋 Key Themes & Highlights
Positive Highlights
- ✅ Single, clear definition of the ASCII alphanumeric character class — eliminates divergence risk
- ✅ Unexported helper keeps the interface surface minimal
- ✅ Both call sites are mechanically correct after the substitution
- ✅ Comment on the helper accurately describes its contract
Minor Observation
The helper isASCIIAlphanumeric is not directly unit-tested; it is exercised indirectly through SanitizeIdentifierName and SanitizeForFilename. That is acceptable for a small private helper, but a direct test would lock in the boundary conditions ('0', '9', 'a', 'z', 'A', 'Z', and one character outside each range) and make the contract self-documenting.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 17.4 AIC · ⌖ 4.54 AIC · ⊞ 6.7K
Comment /matt to run again
|
🎉 This pull request is included in a new release. Release: |
sanitize.gohad two functions open-coding the identical ASCII alphanumeric rune test, creating divergence risk for the character-class definition.Changes
pkg/stringutil/sanitize.go— adds unexported helperisASCIIAlphanumeric(r rune) booland replaces the duplicated inline expressions inSanitizeIdentifierNameandSanitizeForFilename:Purely mechanical, behavior-preserving; existing tests cover both call sites.