refactor: standardize fmt imports and restore submodule separation in proto_ext#53
Conversation
Several protobuf extension types had API gaps: `TxnDeleteByPrefixRequest`, `TxnDeleteResponse`, and `TxnDeleteByPrefixResponse` lacked `new()` constructors that their sibling types already had. `TxnOp` had convenience constructors for every `Request` variant except `DeleteByPrefix`, forcing callers to build the struct manually. Changes: - Add `TxnDeleteByPrefixRequest::new()`, `TxnDeleteResponse::new()`, `TxnDeleteByPrefixResponse::new()` constructors - Add `TxnOp::delete_by_prefix()` to match other `TxnOp` variant constructors - Add test modules to 11 files that had zero test coverage: raft type round-trips, txn request/response constructors and Display impls, and `TxnCondition::Target` Display variants
… proto_ext - Use `use std::fmt;` with `fmt::Display`/`fmt::Formatter` instead of importing individual types - Restore Display impls to separate submodule files (txn_op_ext/request_ext, txn_op_response_ext/response_ext, txn_condition_ext/condition_result_ext, txn_condition_ext/target_ext) - Split raft_types_ext into vote_ext and log_id_ext submodules
xp-trumpet
left a comment
There was a problem hiding this comment.
@xp-trumpet reviewed 24 files and all commit messages.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on @drmingdrmer).
There was a problem hiding this comment.
Pull request overview
This PR refactors the proto_ext module to standardize fmt imports and improve code organization. It changes all files to use use std::fmt; with qualified types instead of importing individual Display/Formatter types. The PR also splits larger modules into submodules (raft_types_ext into vote_ext and log_id_ext) and adds missing constructors and test coverage for several proto types.
Changes:
- Standardized fmt imports across all proto_ext files to use
use std::fmt;pattern - Reorganized module structure: split raft_types_ext into vote_ext and log_id_ext submodules, restored Display impls to separate submodule files
- Added missing
new()constructors for TxnDeleteByPrefixRequest, TxnDeleteResponse, TxnDeleteByPrefixResponse, and TxnOp::delete_by_prefix() - Added comprehensive test coverage (11 files that previously had zero tests)
Reviewed changes
Copilot reviewed 24 out of 24 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| txn_request_ext.rs | Standardized fmt imports |
| txn_reply_ext.rs | Standardized fmt imports |
| txn_put_response_ext.rs | Standardized fmt imports, added tests for Display and unpack methods |
| txn_put_request_ext.rs | Standardized fmt imports, added tests for new() and Display |
| txn_op_response_ext/response_ext.rs | Standardized fmt imports, moved test to parent module, simplified Display match arms |
| txn_op_response_ext.rs | Added test_response_from test (moved from submodule) |
| txn_op_ext/request_ext.rs | Standardized fmt imports, simplified Display match arms |
| txn_op_ext.rs | Added delete_by_prefix() constructor and test |
| txn_delete_response_ext.rs | Standardized fmt imports, added new() constructor and tests |
| txn_delete_request_ext.rs | Standardized fmt imports, added tests for new() and Display |
| txn_delete_by_prefix_response_ext.rs | Standardized fmt imports, added new() constructor and tests |
| txn_delete_by_prefix_request_ext.rs | Standardized fmt imports, added new() constructor and tests |
| txn_condition_ext/target_ext.rs | Standardized fmt imports, simplified Display match arms, added Display tests |
| txn_condition_ext/condition_result_ext.rs | Standardized fmt imports |
| txn_condition_ext.rs | Standardized fmt imports |
| raft_types_ext/vote_ext.rs | New file: Vote conversion implementations with round-trip tests |
| raft_types_ext/log_id_ext.rs | New file: LogId conversion implementations with round-trip test |
| raft_types_ext.rs | Split into vote_ext and log_id_ext submodules |
| mod.rs | Alphabetically sorted module imports |
| vote_response_ext.rs | Added round-trip tests for granted and rejected responses |
| vote_request_ext.rs | Added round-trip tests with and without log_id |
| keys_count_ext.rs | Added tests for new() and Display |
| fetch_increase_u64_response_ext.rs | Standardized fmt imports |
| boolean_expression_ext.rs | Standardized fmt imports |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| pb::TxnDeleteByPrefixRequest { | ||
| prefix: prefix.to_string(), | ||
| }, |
There was a problem hiding this comment.
The delete_by_prefix constructor should use pb::TxnDeleteByPrefixRequest::new(prefix) instead of constructing the struct inline, to be consistent with other constructors in this file like delete_exact (line 85-86) and get (line 94) which use the .new() method.
| pb::TxnDeleteByPrefixRequest { | |
| prefix: prefix.to_string(), | |
| }, | |
| pb::TxnDeleteByPrefixRequest::new(prefix), |
Changelog
refactor: standardize fmt imports and restore submodule separation in proto_ext
use std::fmt;withfmt::Display/fmt::Formatterinstead of importing individual typesfeat: add missing constructors and tests for proto_ext types
Several protobuf extension types had API gaps:
TxnDeleteByPrefixRequest,TxnDeleteResponse, andTxnDeleteByPrefixResponselackednew()constructorsthat their sibling types already had.
TxnOphad convenience constructors forevery
Requestvariant exceptDeleteByPrefix, forcing callers to build thestruct manually.
Changes:
TxnDeleteByPrefixRequest::new(),TxnDeleteResponse::new(),TxnDeleteByPrefixResponse::new()constructorsTxnOp::delete_by_prefix()to match otherTxnOpvariant constructorsround-trips, txn request/response constructors and Display impls, and
TxnCondition::TargetDisplay variants