-
Notifications
You must be signed in to change notification settings - Fork 218
[ISSUE #5409] - common(filter): convert Type enum from Rust #5444
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
🔊@aditya-rajpurohit 🚀Thanks for your contribution🎉! 💡CodeRabbit(AI) will review your code first🔥! Note 🚨The code review suggestions from CodeRabbit are to be used as a reference only, and the PR submitter can decide whether to make changes based on their own judgment. Ultimately, the project management personnel will conduct the final code review💥. |
WalkthroughA new public Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ Finishing touches
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🧹 Nitpick comments (1)
rocketmq-common/src/common/filter/filter_type.rs (1)
15-22: Clean enum implementation; consider optional enhancements.The
FilterTypeenum is well-structured with appropriate derives for a simple categorical type.Optional improvements to consider:
- Add
Hashderive if this enum will be used as a key inHashMaporHashSetcollections (it already hasEq).- Add documentation comments explaining the purpose of each variant (e.g., what constitutes an "Operand" vs "Operator" in the filter context).
Example with Hash and docs
+/// Represents the category of a token in filter expression parsing. -#[derive(Debug, Clone, Copy, PartialEq, Eq)] +#[derive(Debug, Clone, Copy, PartialEq, Eq, Hash)] pub enum FilterType { + /// Null or empty token Null, + /// Operand in an expression (e.g., variable or value) Operand, + /// Operator in an expression (e.g., comparison or logical operator) Operator, + /// Parenthesis for grouping Parenthesis, + /// Separator between tokens Separator, }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
rocketmq-common/src/common/filter.rsrocketmq-common/src/common/filter/filter_type.rs
🧰 Additional context used
🪛 GitHub Actions: RocketMQ Rust CI
rocketmq-common/src/common/filter.rs
[error] 15-15: cargo fmt --all -- --check failed: formatting differences detected in rocketmq-common/src/common/filter.rs. Please run 'cargo fmt' to format the code.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: auto-approve
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #5444 +/- ##
=======================================
Coverage 37.83% 37.83%
=======================================
Files 811 811
Lines 109905 109905
=======================================
Hits 41582 41582
Misses 68323 68323 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
rocketmq-rust-bot
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM - All CI checks passed ✅
mxsm
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Which Issue(s) This PR Fixes
Fixes #5409
Brief Description
This PR converts the common
Typeenum from the Java version of RocketMQ to Rust.Type→FilterTypeto avoid conflicts with Rust keywords and improve clarity.Null,Operand,Operator,Parenthesis,Separator.filtermodule and exposed it infilter.rs.This change is part of the RocketMQ Rust migration effort for the filter module.
How Did You Test This Change?
Verified compilation with
cargo buildSummary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.