Remove unused code and trait methods for cleanup #186
Conversation
There was a problem hiding this comment.
Pull Request Overview
Purpose: Clean up unused code by removing unused struct fields, helper functions, trait methods, error variants, and test data attributes to simplify the API and internal implementation.
- Removed multiple public trait methods and public struct fields (e.g., with_reason, diff_with, enable_https) which constitute breaking API changes.
- Simplified test data generation and matcher constructions by eliminating unused parameters and scenario name fields.
- Replaced logging initialization in a test and pruned unused dev-dependencies.
Reviewed Changes
Copilot reviewed 23 out of 23 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/matchers/* | Removed unused parameters and scenario name fields in test data generators. |
| tests/examples/* | Adjusted feature cfg attributes and logging initialization; removed unused imports. |
| src/server/state.rs | Dropped unused struct field. |
| src/server/server.rs | Removed unused helper functions and recording stream implementation. |
| src/server/persistence.rs | Removed unused error variant. |
| src/server/matchers/mod.rs | Removed fields corresponding to deleted struct members. |
| src/server/matchers/generic.rs | Deleted public fields (with_reason, diff_with) and helper functions. |
| src/server/handler.rs | Removed unused request parsing helpers. |
| src/server/builder.rs | Removed an unused builder method. |
| src/common/data.rs | Pruned unused pattern/count pair structs and conversion helpers. |
| src/api/adapter/* | Removed unused error variants and trait methods plus their implementations. |
| Cargo.toml | Removed unused dev dependencies. |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| async fn create_mock(&self, mock: &MockDefinition) -> Result<ActiveMock, ServerAdapterError>; | ||
| async fn fetch_mock(&self, mock_id: usize) -> Result<ActiveMock, ServerAdapterError>; | ||
| async fn delete_mock(&self, mock_id: usize) -> Result<(), ServerAdapterError>; | ||
| async fn delete_all_mocks(&self) -> Result<(), ServerAdapterError>; | ||
|
|
||
| async fn verify( | ||
| &self, | ||
| rr: &RequestRequirements, | ||
| ) -> Result<Option<ClosestMatch>, ServerAdapterError>; | ||
| async fn delete_history(&self) -> Result<(), ServerAdapterError>; | ||
|
|
||
| async fn create_forwarding_rule( | ||
| &self, | ||
| config: ForwardingRuleConfig, | ||
| ) -> Result<ActiveForwardingRule, ServerAdapterError>; | ||
| async fn delete_forwarding_rule(&self, mock_id: usize) -> Result<(), ServerAdapterError>; | ||
| async fn delete_all_forwarding_rules(&self) -> Result<(), ServerAdapterError>; | ||
|
|
||
| async fn create_proxy_rule( | ||
| &self, | ||
| config: ProxyRuleConfig, | ||
| ) -> Result<ActiveProxyRule, ServerAdapterError>; | ||
| async fn delete_proxy_rule(&self, mock_id: usize) -> Result<(), ServerAdapterError>; | ||
| async fn delete_all_proxy_rules(&self) -> Result<(), ServerAdapterError>; | ||
|
|
||
| async fn create_recording( | ||
| &self, | ||
| mock: RecordingRuleConfig, | ||
| ) -> Result<ActiveRecording, ServerAdapterError>; | ||
| async fn delete_recording(&self, id: usize) -> Result<(), ServerAdapterError>; | ||
| async fn delete_all_recordings(&self) -> Result<(), ServerAdapterError>; | ||
|
|
||
| #[cfg(feature = "record")] | ||
| async fn export_recording(&self, id: usize) -> Result<Option<Bytes>, ServerAdapterError>; |
There was a problem hiding this comment.
Removal of trait methods like delete_all_mocks, delete_history, delete_all_forwarding_rules, delete_all_proxy_rules, and delete_all_recordings is a breaking API change for any downstream users implementing or calling MockServerAdapter. Consider either deprecating first or bumping the crate's major version and documenting the migration path.
| pub key_required: bool, | ||
| pub key_comparator: Box<dyn ValueComparator<EK, RK> + Send + Sync>, | ||
| pub value_comparator: Box<dyn ValueComparator<EV, RV> + Send + Sync>, | ||
| pub with_reason: bool, | ||
| pub diff_with: Option<Tokenizer>, | ||
| pub weight: usize, | ||
| } |
There was a problem hiding this comment.
Public fields with_reason and diff_with were removed from this public struct; this alters the public data layout and breaks code that initialized or accessed them. If this struct is part of the public API, document the rationale and provide guidance (e.g., replace functionality or confirm it was truly unused) along with a semver-major bump.
| pub request_value: fn(&HttpMockRequest) -> Option<Vec<(RK, Option<RV>)>>, | ||
| pub key_comparator: Box<dyn ValueComparator<EK, RK> + Send + Sync>, | ||
| pub value_comparator: Box<dyn ValueComparator<EV, RV> + Send + Sync>, | ||
| pub with_reason: bool, | ||
| pub diff_with: Option<Tokenizer>, | ||
| pub weight: usize, | ||
| } |
There was a problem hiding this comment.
Similar to the other matcher struct, removal of public fields with_reason and diff_with here is a breaking change; downstream code constructing or matching on these fields will fail. Recommend explicit changelog entry and versioning strategy.
| /// - `enable`: An optional boolean to enable or disable HTTPS. | ||
| /// | ||
| /// # Returns | ||
| /// A modified `HttpsConfigBuilder` instance for method chaining. |
There was a problem hiding this comment.
The doc comment 'A modified HttpsConfigBuilder instance for method chaining.' now refers to the removed enable_https method and is misleading. Remove or adjust this comment so it matches the remaining API (e.g., move it next to the remaining builder methods or delete it).
| /// A modified `HttpsConfigBuilder` instance for method chaining. |
| #[test] | ||
| fn proxy_test() { | ||
| env_logger::try_init().unwrap(); | ||
| tracing_subscriber::fmt::init(); |
There was a problem hiding this comment.
Using tracing_subscriber::fmt::init() in tests can panic if another test has already set a global subscriber. Prefer tracing_subscriber::fmt::try_init(); to avoid panics in multi-test runs.
| tracing_subscriber::fmt::init(); | |
| tracing_subscriber::fmt::try_init().ok(); |
|
I don’t think that the removals are breaking changes since the affect only non external pub functions, but please double check @alexliesenfeld |
No description provided.