[ISSUE #1462]🔥Add macro for create client broker error🚀 #1465
Conversation
WalkthroughThe changes introduce a new macro named Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
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 #1465 +/- ##
==========================================
+ Coverage 22.30% 22.33% +0.02%
==========================================
Files 450 450
Lines 58037 58057 +20
==========================================
+ Hits 12948 12966 +18
- Misses 45089 45091 +2 ☔ 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 (1)
rocketmq-client/src/client_error.rs (1)
255-277: Improve test coverage and fix test dataThe test at line 262 uses a raw format string
"{}"as the error message, which seems unintentional. Additionally, the test coverage could be improved.
- Fix the test data:
- client_broker_err!(404, "Error: {}", "127.0.0.1"); + client_broker_err!(404, "Not Found", "127.0.0.1");
- Add tests for edge cases:
#[test] fn client_broker_err_handles_empty_strings() { let result: Result<(), MQClientError> = client_broker_err!(404, "", ""); if let Err(MQClientError::MQClientBrokerError(err)) = result { assert_eq!(err.error_message().unwrap(), ""); assert_eq!(err.broker_addr().unwrap(), ""); } } #[test] fn client_broker_err_handles_special_characters() { let result: Result<(), MQClientError> = client_broker_err!(404, "Error: \n\t\"quoted\"", "localhost:8080"); if let Err(MQClientError::MQClientBrokerError(err)) = result { assert_eq!(err.error_message().unwrap(), "Error: \n\t\"quoted\""); } }
- After implementing format string support, add this test:
#[test] fn client_broker_err_with_format_args_formats_correctly() { let result: Result<(), MQClientError> = client_broker_err!(404, "Error: {} on {}", "Not Found", "Service A", "broker1"); if let Err(MQClientError::MQClientBrokerError(err)) = result { assert_eq!(err.error_message().unwrap(), "Error: Not Found on Service A"); assert_eq!(err.broker_addr().unwrap(), "broker1"); } }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
rocketmq-client/src/client_error.rs(2 hunks)
🔇 Additional comments (1)
rocketmq-client/src/client_error.rs (1)
104-122: Verify macro usage in the codebase
Let's verify how this macro will be used and ensure there are no breaking changes.
✅ Verification successful
Macro implementation aligns with existing error handling patterns
The verification shows that the macro implementation perfectly matches the existing error handling patterns in the codebase:
- The macro's two variants (
new_with_brokerandnew) align with actual usage patterns found in multiple files:mq_client_api_impl.rsuses the three-argument pattern (code, message, broker_addr)pull_callback.rsdemonstrates error code checking from broker errors- Test cases in
client_error.rsvalidate both variants
The macro is a type-safe wrapper that standardizes the existing error creation patterns without introducing breaking changes.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for potential usage patterns of broker errors in the codebase
# This will help understand how the macro should be implemented
# Look for existing broker error creation patterns
echo "Checking existing broker error patterns..."
rg -A 2 "MQClientError::MQClientBrokerError" --type rust
# Look for potential usage patterns in comments or TODOs
echo "Checking potential usage patterns..."
rg -A 2 "broker.*error" --type rust
Length of output: 10990
| #[macro_export] | ||
| macro_rules! client_broker_err { | ||
| // Handle errors with a custom ResponseCode and formatted string | ||
| ($response_code:expr, $error_message:expr, $broker_addr:expr) => {{ | ||
| std::result::Result::Err($crate::client_error::MQClientError::MQClientBrokerError( | ||
| $crate::client_error::MQBrokerErr::new_with_broker( | ||
| $response_code as i32, | ||
| $error_message, | ||
| $broker_addr, | ||
| ), | ||
| )) | ||
| }}; | ||
| // Handle errors without a ResponseCode, using only the error message | ||
| ($response_code:expr, $error_message:expr) => {{ | ||
| std::result::Result::Err($crate::client_error::MQClientError::MQClientBrokerError( | ||
| $crate::client_error::MQBrokerErr::new($response_code as i32, $error_message), | ||
| )) | ||
| }}; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add documentation and support format strings
The macro implementation could be improved in several ways:
- Missing documentation comments explaining usage and examples
- Unlike
mq_client_err!andrequest_timeout_err!, this macro doesn't support format strings with multiple arguments
Consider adding documentation and extending the macro like this:
#[macro_export]
+/// Creates a broker-related error result
+///
+/// # Examples
+///
+/// ```
+/// let err = client_broker_err!(404, "Error: {} - {}", "Not Found", "broker1");
+/// let err = client_broker_err!(404, "Not Found");
+/// ```
macro_rules! client_broker_err {
+ // Handle errors with formatted string and broker address
+ ($response_code:expr, $fmt:expr, $($arg:expr),*, $broker_addr:expr) => {{
+ let formatted_msg = format!($fmt, $($arg),*);
+ std::result::Result::Err($crate::client_error::MQClientError::MQClientBrokerError(
+ $crate::client_error::MQBrokerErr::new_with_broker(
+ $response_code as i32,
+ formatted_msg,
+ $broker_addr,
+ ),
+ ))
+ }};
// Handle errors with a custom ResponseCode and formatted string
($response_code:expr, $error_message:expr, $broker_addr:expr) => {{Committable suggestion skipped: line range outside the PR's diff.
rocketmq-rust-bot
added
approved
Which Issue(s) This PR Fixes(Closes)
Fixes #1462
Brief Description
How Did You Test This Change?
Summary by CodeRabbit
New Features
client_broker_err!, for easier error handling related to broker errors.Tests