[ISSUE #1460]🔥Add macro for create client error🚀 #1461
Conversation
WalkthroughThe changes introduce a new macro, Changes
Assessment against linked issues
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 ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1461 +/- ##
==========================================
+ Coverage 22.24% 22.28% +0.03%
==========================================
Files 450 450
Lines 57982 58011 +29
==========================================
+ Hits 12899 12925 +26
- Misses 45083 45086 +3 ☔ 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: 1
🧹 Outside diff range and nitpick comments (3)
rocketmq-client/src/client_error.rs (3)
144-157: Add documentation for the macroThe macro implementation looks good, but it would benefit from documentation explaining its usage patterns and examples.
Add documentation like this:
+/// Creates a MQClientError with optional response code and formatted message. +/// +/// # Examples +/// +/// ``` +/// // With response code and formatted message +/// let err = mq_client_err!(404, "Resource {} not found", "users"); +/// +/// // With just error message +/// let err = mq_client_err!("Operation failed"); +/// ``` #[macro_export] macro_rules! mq_client_err {
151-151: Make the error path more explicitThe error construction could be more explicit about the types being used.
Consider this improvement:
- std::result::Result::Err($crate::client_error::MQClientError::MQClientErr( ClientErr::new_with_code($response_code as i32, formatted_msg))) + std::result::Result::Err($crate::client_error::MQClientError::MQClientErr( + $crate::client_error::ClientErr::new_with_code($response_code as i32, formatted_msg) + ))
204-243: Add more test coverage for edge casesThe current tests cover the basic functionality well, but consider adding these test cases:
#[test] fn mq_client_err_with_negative_response_code() { let result = mq_client_err!(-100, "Negative code: {}", "test"); if let Err(MQClientError::MQClientErr(err)) = result { assert_eq!(err.response_code(), -100); } } #[test] fn mq_client_err_verifies_faq_url_attachment() { let result = mq_client_err!("test error"); if let Err(MQClientError::MQClientErr(err)) = result { // Verify the FAQ URL is properly attached to the message assert!(format!("{}", err).contains("FAQUrl")); } }
| // Create a macro to simplify error creation | ||
| #[macro_export] | ||
| macro_rules! mq_client_err { | ||
| // Handle errors with a custom ResponseCode and formatted string | ||
| ($response_code:expr, $fmt:expr, $($arg:expr),*) => {{ | ||
| let formatted_msg = format!($fmt, $($arg),*); | ||
|
|
||
| std::result::Result::Err($crate::client_error::MQClientError::MQClientErr( ClientErr::new_with_code($response_code as i32, formatted_msg))) | ||
| }}; | ||
| // Handle errors without a ResponseCode, using only the error message | ||
| ($error_message:expr) => {{ | ||
| std::result::Result::Err($crate::client_error::MQClientError::MQClientErr(ClientErr::new($error_message))) | ||
| }}; | ||
| } |
There was a problem hiding this comment.
💡 Codebase verification
Consider migrating existing error creation patterns to use the new macro
The search results show extensive direct usage of ClientErr::new() and ClientErr::new_with_code() across the codebase that could benefit from the new macro:
- Found ~60 instances of direct
ClientErr::new()calls - Found ~25 instances of direct
ClientErr::new_with_code()calls - Most usages follow patterns that match the macro's two variants:
- Simple error messages:
ClientErr::new("message") - Formatted messages with response codes:
ClientErr::new_with_code(code, format!("msg {}", var))
- Simple error messages:
Suggested locations to migrate:
rocketmq-client/src/base/validators.rs: Contains multiple simple error creationsrocketmq-client/src/producer/producer_impl/default_mq_producer_impl.rs: Has many formatted error messagesrocketmq-client/src/consumer/consumer_impl/default_mq_push_consumer_impl.rs: Contains numerous error creations with formatting
🔗 Analysis chain
Verify existing error creation patterns
Let's check if there are existing error creation patterns that could be migrated to use this new macro.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find existing error creation patterns that could be migrated
# Look for direct ClientErr::new calls
echo "Finding direct ClientErr::new calls:"
rg "ClientErr::new\(" --type rust
# Look for direct ClientErr::new_with_code calls
echo -e "\nFinding direct ClientErr::new_with_code calls:"
rg "ClientErr::new_with_code\(" --type rust
# Look for MQClientError::MQClientErr construction
echo -e "\nFinding MQClientError::MQClientErr construction:"
rg "MQClientError::MQClientErr\(" --type rust
Length of output: 22690
rocketmq-rust-bot
added
approved
Which Issue(s) This PR Fixes(Closes)
Fixes #1460
Brief Description
How Did You Test This Change?
Summary by CodeRabbit
New Features
mq_client_err!, to simplify error handling by streamlining the creation ofMQClientErrorinstances.Tests
mq_client_err!macro, ensuring correct error message formatting and response code assignment.