[ISSUE #4087]♻️Clean up existing clippy warnings of useless_vec, redundant_field_names, let_unit_value, manual_range_contains#4106
Conversation
rocketmq-rust-bot
requested review from
SpaceXCN,
TeslaRustor and
rocketmq-rust-bot
|
Warning Rate limit exceeded@WaterWhisperer has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 14 minutes and 55 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (3)
WalkthroughSmall, test-only changes: adjusted variable moves/clones and added a JSON equality check; replaced Changes
Sequence Diagram(s)Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
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 |
|
🔊@WaterWhisperer 🚀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💥. |
rocketmq-rust-bot
added
AI review first
There was a problem hiding this comment.
Pull Request Overview
This PR cleans up clippy warnings in the Rust codebase by addressing four specific lint categories: useless_vec, redundant_field_names, let_unit_value, and manual_range_contains.
- Replaced
vec![]with slice references (&[]) in test functions where vector ownership isn't needed - Removed redundant field names in struct initialization where field and variable names match
- Fixed unit value assignments by properly awaiting async operations and removing unnecessary assignments
- Replaced manual range checks with the more idiomatic
contains()method on range expressions
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| rocketmq/src/schedule.rs | Fixed unit value assignments and manual range contains checks in tests |
| rocketmq-remoting/src/protocol/header/extra_info_util.rs | Replaced useless vec with slice references in test function calls |
| rocketmq-remoting/src/protocol/body/cluster_acl_version_info.rs | Removed redundant field names in struct initialization |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4106 +/- ##
==========================================
+ Coverage 26.55% 26.59% +0.03%
==========================================
Files 575 575
Lines 81356 81361 +5
==========================================
+ Hits 21606 21636 +30
+ Misses 59750 59725 -25 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
rocketmq-remoting/src/protocol/header/extra_info_util.rs (1)
310-316: Builder vs. parser delimiter mismatch (semicolon vs. comma) — potential parse failure
build_msg_offset_infojoins offsets with;, whileparse_msg_offset_infoexpects offsets joined by,and also uses;to split records. Strings produced by the builder won’t round-trip through the parser and may be split incorrectly.Apply this diff to emit comma-separated offsets from the builder (keeps
;for record separation used by the parser):- for (i, offset) in msg_offsets.iter().enumerate() { - string_builder.push_str(&offset.to_string()); - if i < msg_offsets.len() - 1 { - string_builder.push(';'); - } - } + for (i, offset) in msg_offsets.iter().enumerate() { + if i > 0 { + string_builder.push(','); + } + string_builder.push_str(&offset.to_string()); + }And update the corresponding test expectation:
- assert_eq!(string_builder, "0 7 100;200;300"); + assert_eq!(string_builder, "0 7 100,200,300");If this is out of scope for this PR, please file a follow-up and link it to avoid future integration bugs.
Also applies to: 346-352
🧹 Nitpick comments (2)
rocketmq-remoting/src/protocol/body/cluster_acl_version_info.rs (1)
64-83: “equality” test doesn’t assert anythingThe test constructs two values but never compares them. Add a concrete assertion to validate equivalence.
Apply this diff to assert via JSON (avoids trait bounds on DataVersion):
#[test] fn cluster_acl_version_info_equality() { @@ - let _info1 = ClusterAclVersionInfo { + let info1 = ClusterAclVersionInfo { @@ - let _info2 = ClusterAclVersionInfo { + let info2 = ClusterAclVersionInfo { @@ - }; + }; + + assert_eq!( + serde_json::to_string(&info1).unwrap(), + serde_json::to_string(&info2).unwrap() + ); }rocketmq/src/schedule.rs (1)
750-752: Awaiting sleep + shorter window improves determinismGood catch adding
.awaitand trimming the delay; this prevents a no-op sleep and speeds up CI.For consistency with the rest of the module (which imports
tokio::time as time), consider usingtime::sleep:- tokio::time::sleep(Duration::from_millis(400)).await; + time::sleep(Duration::from_millis(400)).await;
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
rocketmq-remoting/src/protocol/body/cluster_acl_version_info.rs(2 hunks)rocketmq-remoting/src/protocol/header/extra_info_util.rs(21 hunks)rocketmq/src/schedule.rs(3 hunks)
⏰ 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). (9)
- GitHub Check: build (ubuntu-latest, stable)
- GitHub Check: build (windows-latest, nightly)
- GitHub Check: build (windows-latest, stable)
- GitHub Check: build (ubuntu-latest, nightly)
- GitHub Check: build (macos-latest, stable)
- GitHub Check: build (macos-latest, nightly)
- GitHub Check: test
- GitHub Check: build
- GitHub Check: auto-approve
🔇 Additional comments (5)
rocketmq-remoting/src/protocol/body/cluster_acl_version_info.rs (2)
80-82: Field init shorthand is correct and reduces redundancy.Using
all_acl_config_data_version,here is idiomatic and preserves the intended move semantics.
109-115: Same here: good use of field init shorthand.No behavior change; the map is moved as intended.
rocketmq/src/schedule.rs (2)
809-812: Replaced manual bounds check with range.containsThis aligns with Clippy’s
manual_range_containsguidance and reads better.
838-841: Same improvement hereCleaner and idiomatic containment check.
rocketmq-remoting/src/protocol/header/extra_info_util.rs (1)
501-927: Tests now pass slices instead of Vec — LGTMThe slice-based calls remove
useless_vecoverhead and match the function signatures. No functional change.
WaterWhisperer
force-pushed
the
refactor-issueNo4087
branch
from
1bfd8cc to
2721886
Compare
|
🔊@WaterWhisperer 🚀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💥. |
WaterWhisperer
force-pushed
the
refactor-issueNo4087
branch
from
2721886 to
b20f4af
Compare
|
🔊@WaterWhisperer 🚀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💥. |
- useless_vec - redundant_field_names - let_unit_value - manual_range_contains
WaterWhisperer
force-pushed
the
refactor-issueNo4087
branch
from
b20f4af to
c3e8ee4
Compare
|
🔊@WaterWhisperer 🚀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💥. |
rocketmq-rust-bot
added
approved
Which Issue(s) This PR Fixes(Closes)
#4087 Not complete yet
Brief Description
Clean up existing clippy warnings of types:
How Did You Test This Change?
Summary by CodeRabbit