fix: ensure SerializableStopBy serialization matches deserialization …#1804
fix: ensure SerializableStopBy serialization matches deserialization …#1804HerringtonDarkholme merged 1 commit intoast-grep:mainfrom leon-rs:fix-serializable-bug
Conversation
WalkthroughThe code changes update the serialization logic for the Changes
Sequence Diagram(s)sequenceDiagram
participant S as Serializer
participant E as SerializableStopBy
participant C as Constants (NEIGHBOR_KEY / END_KEY)
S->>E: Call serialize(serializer)
E->>C: Retrieve appropriate constant for variant (Neighbor/End)
C-->>E: Return string key
E->>S: Return serialized string key
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
crates/config/src/rule/stop_by.rs (2)
24-27: LGTM! Good use of constants for string literals.Using constants instead of string literals improves maintainability and reduces the risk of typos.
Consider adding a doc comment explaining the purpose of these constants:
impl SerializableStopBy { + /// String key used for serializing the Neighbor variant const NEIGHBOR_KEY: &str = "neighbor"; + /// String key used for serializing the End variant const END_KEY: &str = "end"; }
41-45: Use constants in error message for consistency.The error message still uses string literals while the match arms use constants.
SerializableStopBy::NEIGHBOR_KEY => Ok(SerializableStopBy::Neighbor), SerializableStopBy::END_KEY => Ok(SerializableStopBy::End), v => Err(de::Error::custom(format!( - "unknown variant `{v}`, expected `neighbor`, `end` or a rule object", + "unknown variant `{v}`, expected `{}`, `{}` or a rule object", + SerializableStopBy::NEIGHBOR_KEY, + SerializableStopBy::END_KEY, ))),
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
crates/config/src/rule/stop_by.rs(3 hunks)
🔇 Additional comments (2)
crates/config/src/rule/stop_by.rs (2)
16-22: LGTM! Removal of camelCase renaming aligns with serialization fix.The removal of the
rename_allattribute is appropriate as we're now handling serialization explicitly through a custom implementation.
67-78:Details
❓ Verification inconclusive
LGTM! Clean serialization implementation that matches deserialization.
The implementation correctly handles all variants and maintains consistency with the deserialization logic.
Consider adding round-trip serialization tests to verify the fix. Here's a script to check for existing tests:
🏁 Script executed:
#!/bin/bash # Description: Check for round-trip serialization tests rg -A 5 'test.*serializ' crates/config/src/rule/stop_by.rsLength of output: 60
LGTM! The custom serialization implementation for
SerializableStopByis clean and properly delegates each variant. However, the recent check did not reveal any existing round-trip serialization tests in the vicinity ofcrates/config/src/rule/stop_by.rs. Please manually verify if such tests exist elsewhere or consider adding round-trip tests to ensure that every variant serializes and deserializes correctly.
|
Can you also add a test for this? Thanks |
|
Also the test failed, can you give it a look? |
…1802) SerializableStopBy previously had a custom deserialization implementation, but relied on Serde's derive macro for serialization. This caused issues when round-tripping YAML configurations. This commit implements a custom Serialize trait to ensure that: - "neighbor" and "end" are serialized as strings. - Rule variants are serialized as maps. Fixes #1802.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1804 +/- ##
==========================================
+ Coverage 87.36% 87.43% +0.06%
==========================================
Files 96 96
Lines 15528 15553 +25
==========================================
+ Hits 13566 13598 +32
+ Misses 1962 1955 -7 ☔ View full report in Codecov by Sentry. |
…(#1802)
SerializableStopBy previously had a custom deserialization implementation, but relied on Serde's derive macro for serialization. This caused issues when round-tripping YAML configurations.
This commit implements a custom Serialize trait to ensure that:
Fixes #1802.
Summary by CodeRabbit
Refactor
SerializableStopByenum has been enhanced for clarity and maintainability, improving the handling of string keys associated with enum variants. This update contributes to a more robust configuration processing experience.Tests