Repository navigation
Derive the transition effect count and variant inventory - #496
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configuration
📒 Files selected for processing (1)
Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 2 remain after this review. 📝 WalkthroughWalkthrough
ChangesTransitionEffect metadata
Priority: ⬇️ Low Estimated code review effort: 2 (Simple) | ~5 minutes Change: Refactor Merge Risk: ⚪ Minimal · up to This refactor derives the transition effect list and count from the enum definition without changing the public API shape. No actionable merge-blocking risk was found. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
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 |
There was a problem hiding this comment.
🟢 Approval recommended
The change is a small, self-contained refactor that preserves the public API, uses an established codebase pattern, is valid const Rust (enum is Copy), and is covered by existing tests for ALL and COUNT.
0 open findings
What changed in this PR
This PR refactors the TransitionEffect enum in scroll_animation.rs to derive its variant count (COUNT) and inventory (ALL) from the strum::EnumCount and strum::VariantArray macros, instead of maintaining them by hand. This removes a maintenance footgun where adding a new transition variant previously required manually updating the hardcoded COUNT = 21 and the 21-entry ALL array. The public API (COUNT, the fixed-size ALL array, parsing aliases, and serialization) is preserved, and no new dependencies are introduced since strum with the derive feature is already a workspace dependency.
Changes:
- Added
strum::EnumCountandstrum::VariantArrayto the enum's derives. - Replaced the hardcoded
COUNT = 21with<Self as strum::EnumCount>::COUNT. - Replaced the handwritten 21-entry
ALLarray with a const loop that copiesstrum's derivedVARIANTSinto the fixed-size array.
| File | Description |
|---|---|
| crates/neomacs-display-protocol/src/scroll_animation.rs | Derives COUNT and ALL for TransitionEffect via strum macros, removing the manually maintained count and variant list while preserving the public fixed-size array and ordering. |
🧠 Review effort: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Adding a
TransitionEffectcurrently requires updating both the enum declaration and its handwrittenCOUNTandALLinventory. Derive the inventory with the existingstrum::EnumCountandstrum::VariantArraymacros so new variants are included automatically.Preserve the public fixed-size
ALLarray, declaration order, parsing aliases, canonical names, and serialization. No dependencies are added.Validation:
cargo test --locked -p neomacs-display-protocol --lib: all 851 tests passed.ALLarray: ordering is unchanged.git diff --checkpassed.