[ISSUE #1658]🚀Add AckResult struct🔥 #1661
Conversation
WalkthroughThe changes introduce a new public module named Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
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
Documentation and Community
|
|
🔊@mxsm 🚀Thanks for your contribution 🎉. CodeRabbit(AI) will review your code first 🔥 |
rocketmq-rust-bot
added
AI review first
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1661 +/- ##
==========================================
+ Coverage 27.49% 27.55% +0.05%
==========================================
Files 467 468 +1
Lines 62617 62668 +51
==========================================
+ Hits 17216 17267 +51
Misses 45401 45401 ☔ View full report in Codecov by Sentry. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
rocketmq-client/src/consumer/ack_result.rs (2)
21-26: Add documentation and consider builder patternThe struct definition would benefit from:
- Documentation explaining the purpose of each field, especially
pop_time- A builder pattern implementation for more flexible object construction
Add documentation and implement builder:
#[derive(Debug, Clone)] +/// Represents the result of an acknowledgment operation pub struct AckResult { + /// The status of the acknowledgment operation pub(crate) status: AckStatus, + /// Additional information about the acknowledgment pub(crate) extra_info: CheetahString, + /// Timestamp when the message was popped/consumed pub(crate) pop_time: i64, } +impl AckResult { + pub fn builder() -> AckResultBuilder { + AckResultBuilder::default() + } +} +#[derive(Default)] +pub struct AckResultBuilder { + status: Option<AckStatus>, + extra_info: Option<CheetahString>, + pop_time: Option<i64>, +} +impl AckResultBuilder { + pub fn status(mut self, status: AckStatus) -> Self { + self.status = Some(status); + self + } + + pub fn extra_info(mut self, info: impl Into<CheetahString>) -> Self { + self.extra_info = Some(info.into()); + self + } + + pub fn pop_time(mut self, time: i64) -> Self { + self.pop_time = Some(time); + self + } + + pub fn build(self) -> Result<AckResult, &'static str> { + Ok(AckResult { + status: self.status.ok_or("status is required")?, + extra_info: self.extra_info.unwrap_or_default(), + pop_time: self.pop_time.unwrap_or_default(), + }) + } +}
38-95: Enhance test coverageWhile the current tests are good, consider adding:
- Tests for Clone implementation
- Edge cases for
pop_time(0, i64::MAX)- Tests with different CheetahString content types (special characters, multi-byte)
Add these additional tests:
#[test] fn test_clone() { let original = AckResult { status: AckStatus::Ok, extra_info: CheetahString::from("test"), pop_time: 123, }; let cloned = original.clone(); assert_eq!(format!("{}", original), format!("{}", cloned)); } #[test] fn test_pop_time_edge_cases() { let zero_time = AckResult { status: AckStatus::Ok, extra_info: CheetahString::from("test"), pop_time: 0, }; let max_time = AckResult { status: AckStatus::Ok, extra_info: CheetahString::from("test"), pop_time: i64::MAX, }; // Add assertions based on your Display implementation } #[test] fn test_special_characters() { let result = AckResult { status: AckStatus::Ok, extra_info: CheetahString::from("测试€$@"), pop_time: 123, }; assert_eq!( format!("{}", result), "AckResult [AckStatus=Ok, extraInfo=测试€$@]" ); }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
rocketmq-client/src/consumer.rs(1 hunks)rocketmq-client/src/consumer/ack_result.rs(1 hunks)
🔇 Additional comments (2)
rocketmq-client/src/consumer.rs (1)
17-17: LGTM! Module declaration follows existing patterns
The new ack_result module declaration maintains consistent visibility (pub(crate)) and organization with other similar modules.
rocketmq-client/src/consumer/ack_result.rs (1)
28-36: Verify pop_time exclusion from Display implementation
The Display implementation excludes the pop_time field. If this is intentional, please document this decision. If not, consider including it in the formatted output.
Consider adding a comment explaining why pop_time is excluded or update the implementation:
impl std::fmt::Display for AckResult {
+ /// Formats the AckResult, intentionally excluding pop_time for clarity
fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
write!(
f,
- "AckResult [AckStatus={:?}, extraInfo={}]",
- self.status, self.extra_info
+ "AckResult [AckStatus={:?}, extraInfo={}, popTime={}]",
+ self.status, self.extra_info, self.pop_time
)
}
}
rocketmq-rust-bot
added
approved
Which Issue(s) This PR Fixes(Closes)
Fixes #1658
Brief Description
How Did You Test This Change?
Summary by CodeRabbit
New Features
ack_resultfor better organization of related functionalities.AckResultto enhance acknowledgment handling, including status and additional information.AckResultto improve readability in outputs.Tests
Displayimplementation forAckResult.