CLAUDE.md: the serializer owns the wire format - #2367
Conversation
Three packets already declare EscapeSpaces for a last field that is a value
rather than free text, and the fourth was about to get a Replace(' ', '^')
at the call site instead - the second time that shortcut has been proposed.
Write the rule down so the next one goes to the package.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
Included review availability: Your plan provides up to 2 included reviews per hour; 1 remains after this review. WalkthroughThe change adds a code convention that assigns wire-format ownership to the serializer. It prohibits value transformations for packet formatting and directs escaping fixes to ChangesSerializer conventions
Estimated code review effort: 1 (Trivial) | ~3 minutes Merge Risk: ⚪ Minimal · up to This localized documentation change clarifies that wire-format handling belongs in the serializer and does not alter runtime behavior; no actionable merge-blocking risk remains after normal checks and review. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Full details: Docstring CoverageExplanation No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 0 files. (1 skipped: 1 unsupported.) ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
What
One rule under Code conventions, next to Keep semantic types — the same class of mistake, one level further along:
Why
GInfoPacket,MlInfoBrPacketandMlintroPacketalready declareEscapeSpacesfor a last field that carries a value rather than free text.EInfoNpcMonsterPacket.Namedid not, and #2365 was about to shipReplace(' ', '^')at the call site to compensate — knowingly, with a comment saying the flag belonged in the package. That shortcut is easy to reach for because it works, and it leaves the wire knowledge in two repos.The packet side is fixed in NosCoreIO/NosCore.Packets#509, released as 21.1.2.
Summary by CodeRabbit