Conversation
📝 WalkthroughWalkthroughThis diff replaces positional format placeholders with named placeholders across benchmarks, examples, tests, build scripts, and core source files; no logic, control flow, or public APIs were modified. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
f904140 to
ad126a6
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
harness/tests/integration_cases/test_raft.rs (1)
452-501:⚠️ Potential issue | 🟠 MajorResolve remaining
clippy::useless_vecfailures in fixed test tablesCI is still failing because these test-case collections are fixed-size and should be arrays, not
vec![]. This currently blocks the PR objective (“fix clippy warnings”).Suggested fix
- let mut tests = vec![ + let mut tests = [ ( Network::new_with_config(vec![None, None, None], &config, l), StateRole::Leader, 1, ), ... - ]; + ]; - let mut tests = vec![ + let mut tests = [ ( Network::new(vec![None, None, None], &l), vec![new_message(1, 1, MessageType::MsgPropose, 1)], 2, ), ... - ]; + ]; - let mut tests = vec![ + let mut tests = [ Network::new(vec![None, None, None], &l), Network::new(vec![None, None, NOP_STEPPER], &l), - ]; + ];Also applies to: 766-781, 1118-1121
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@harness/tests/integration_cases/test_raft.rs` around lines 452 - 501, The tests variable uses fixed-size collections created with vec![] which triggers clippy::useless_vec; change those test-case lists to fixed-size arrays (replace vec![...] with [...] and adjust types if necessary) for each occurrence where Network::new_with_config is called (including the block building tests and the similar blocks at the other locations noted), leaving constructors like Network::new_with_config(..., &config, l), NOP_STEPPER, and StateRole entries unchanged; ensure the surrounding variable/type declarations accept arrays (or use slices) so the test table compiles without heap-allocated Vecs.src/tracker/progress.rs (1)
297-309:⚠️ Potential issue | 🟠 MajorCI blocker:
clippy::useless_vecis still present in this test table.Pipeline reports this at Line 297-Line 308. Since this PR targets Clippy cleanup, this fixture should be an array literal to clear the lint and unblock CI.
♻️ Proposed fix
- let mut tests = vec![ + let mut tests = [ ( new_progress(ProgressState::Replicate, matched, 5, 0, 256), 2, ), // snapshot finish ( new_progress(ProgressState::Snapshot, matched, 5, 10, 256), 11, ), // snapshot failure (new_progress(ProgressState::Snapshot, matched, 5, 0, 256), 2), - ]; + ];🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tracker/progress.rs` around lines 297 - 309, Replace the temporary Vec test fixture with an array literal to satisfy clippy::useless_vec: change the declaration of tests (currently let mut tests = vec![ ... ]) to an array (for example let tests = [ (new_progress(ProgressState::Replicate, matched, 5, 0, 256), 2), (new_progress(ProgressState::Snapshot, matched, 5, 10, 256), 11), (new_progress(ProgressState::Snapshot, matched, 5, 0, 256), 2) ];), remove the unnecessary mut if tests is not mutated, and keep the same tuple elements that reference new_progress and ProgressState so behavior is unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@harness/tests/integration_cases/test_raft.rs`:
- Around line 452-501: The tests variable uses fixed-size collections created
with vec![] which triggers clippy::useless_vec; change those test-case lists to
fixed-size arrays (replace vec![...] with [...] and adjust types if necessary)
for each occurrence where Network::new_with_config is called (including the
block building tests and the similar blocks at the other locations noted),
leaving constructors like Network::new_with_config(..., &config, l),
NOP_STEPPER, and StateRole entries unchanged; ensure the surrounding
variable/type declarations accept arrays (or use slices) so the test table
compiles without heap-allocated Vecs.
In `@src/tracker/progress.rs`:
- Around line 297-309: Replace the temporary Vec test fixture with an array
literal to satisfy clippy::useless_vec: change the declaration of tests
(currently let mut tests = vec![ ... ]) to an array (for example let tests = [
(new_progress(ProgressState::Replicate, matched, 5, 0, 256), 2),
(new_progress(ProgressState::Snapshot, matched, 5, 10, 256), 11),
(new_progress(ProgressState::Snapshot, matched, 5, 0, 256), 2) ];), remove the
unnecessary mut if tests is not mutated, and keep the same tuple elements that
reference new_progress and ProgressState so behavior is unchanged.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (19)
benches/suites/raft.rsdatadriven/src/line_sparser.rsexamples/five_mem_node/main.rsharness/tests/integration_cases/test_raft.rsharness/tests/integration_cases/test_raft_paper.rsharness/tests/integration_cases/test_raw_node.rsharness/tests/test_util/mod.rsproto/build.rsproto/src/confchange.rssrc/confchange/changer.rssrc/confchange/datadriven_test.rssrc/config.rssrc/quorum.rssrc/quorum/datadriven_test.rssrc/raft.rssrc/raft_log.rssrc/storage.rssrc/tracker/progress.rssrc/util.rs
mostly generated by ``` cargo clippy --all --all-targets --fix ``` Signed-off-by: Guillaume Girol <guillaume.girol@rubycat.eu>
ad126a6 to
23e9ed8
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/config.rs (1)
186-189: Consider applying the same named placeholder style for consistency.This nearby error message still uses positional placeholders while line 194 now uses named placeholders. For consistency within the
validatefunction, consider updating this as well.♻️ Optional consistency fix
if min_timeout < self.election_tick { return Err(Error::ConfigInvalid(format!( - "min election tick {} must not be less than election_tick {}", - min_timeout, self.election_tick + "min election tick {min_timeout} must not be less than election_tick {}", + self.election_tick ))); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/config.rs` around lines 186 - 189, The error message in validate is using positional placeholders; change it to named placeholders to match the style used elsewhere: update the ConfigInvalid message in the branch that returns Err(Error::ConfigInvalid(...)) so it references {min_timeout} and {election_tick} by name (using the same identifiers min_timeout and self.election_tick) instead of positional formatting, ensuring consistency with the other named-placeholder messages in the validate function.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/config.rs`:
- Around line 186-189: The error message in validate is using positional
placeholders; change it to named placeholders to match the style used elsewhere:
update the ConfigInvalid message in the branch that returns
Err(Error::ConfigInvalid(...)) so it references {min_timeout} and
{election_tick} by name (using the same identifiers min_timeout and
self.election_tick) instead of positional formatting, ensuring consistency with
the other named-placeholder messages in the validate function.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (19)
benches/suites/raft.rsdatadriven/src/line_sparser.rsexamples/five_mem_node/main.rsharness/tests/integration_cases/test_raft.rsharness/tests/integration_cases/test_raft_paper.rsharness/tests/integration_cases/test_raw_node.rsharness/tests/test_util/mod.rsproto/build.rsproto/src/confchange.rssrc/confchange/changer.rssrc/confchange/datadriven_test.rssrc/config.rssrc/quorum.rssrc/quorum/datadriven_test.rssrc/raft.rssrc/raft_log.rssrc/storage.rssrc/tracker/progress.rssrc/util.rs
✅ Files skipped from review due to trivial changes (1)
- src/storage.rs
🚧 Files skipped from review as they are similar to previous changes (14)
- src/raft_log.rs
- src/raft.rs
- src/confchange/changer.rs
- benches/suites/raft.rs
- harness/tests/integration_cases/test_raw_node.rs
- harness/tests/integration_cases/test_raft.rs
- proto/build.rs
- harness/tests/integration_cases/test_raft_paper.rs
- examples/five_mem_node/main.rs
- src/quorum.rs
- proto/src/confchange.rs
- src/util.rs
- src/quorum/datadriven_test.rs
- datadriven/src/line_sparser.rs
|
Thank you! |
generated by
Summary by CodeRabbit