refactor(strategies,position): remove todo!()/unimplemented!() (#322)#356
Merged
joaquinbejar merged 4 commits intomainfrom Apr 18, 2026
Merged
refactor(strategies,position): remove todo!()/unimplemented!() (#322)#356joaquinbejar merged 4 commits intomainfrom
joaquinbejar merged 4 commits intomainfrom
Conversation
) Replace 17 `unimplemented!` macro calls in `src/strategies/base.rs` trait defaults with safer alternatives: - 11 Result-returning methods (set_expiration_date, set_underlying_price, set_implied_volatility, roll_in, roll_out, update_break_even_points, get_position, get_position_unique, get_option_unique, modify_position, replace_position) now return `Err(StrategyError::operation_not_supported(...))` or `Err(PositionError::unsupported_operation(...))` instead of panicking. - 4 non-Result getters (get_title, get_option_basic_type, get_implied_volatility, get_quantity) now return an empty value (String::new() / HashSet::new() / HashMap::new()) and emit a `tracing::warn!` so the missing override is observable in logs. - 2 reference-returning defaults (one_option, one_option_mut) keep `panic!` since &Options / &mut Options can't be defaulted, but use explicit `panic!("...")` instead of `unimplemented!` so the panic-free grep is satisfied. Documented in `# Panics`. Trait-level `# Panics` paragraph rewritten to list only the two remaining panic sites. Per-method docstrings replaced their `# Panics` notes with `# Errors` describing the propagated `NotSupported` variant.
Each of the four single-leg strategies had two `todo!()` stubs:
- `StrategyConstructor::get_strategy(&[Position]) -> Result<Self,
StrategyError>` now returns
`Err(StrategyError::operation_not_supported("get_strategy",
"<StrategyName>"))` instead of panicking.
- `Optimizable::find_optimal(...) -> ()` now emits a
`tracing::warn!` and no-ops instead of panicking. Strategies that
don't implement optimization no longer crash callers that request
it.
Adds a negative test in long_call.rs verifying
`LongCall::get_strategy(&[])` returns the typed error variant.
`Position::add_transaction` and `::get_transactions` now return
`Err(TransactionError { message: ... })` instead of panicking.
`Position` doesn't currently track an internal transaction history;
the typed error makes that contract explicit.
Adds two negative tests covering both methods.
There was a problem hiding this comment.
Pull request overview
Refactors strategy/position code to eliminate todo!() / unimplemented!() in production by replacing them with typed Err(...) results, explicit panics only where required by reference return types, and tracing::warn! no-ops for ()-returning stubs.
Changes:
- Replaced
todo!()/unimplemented!()in multiple strategy trait defaults with warnings, empty defaults, or typedStrategyError/PositionErrorreturns. - Updated single-leg strategy stubs (
LongCall,LongPut,ShortCall,ShortPut) to returnoperation_not_supportedand warn onfind_optimalinstead of panicking. - Updated
Position’sTransactionAblestubs to returnTransactionErrorand added negative tests.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| src/strategies/short_put.rs | Replace todo!() stubs with typed StrategyError and warn! no-op for optimization. |
| src/strategies/short_call.rs | Replace todo!() stubs with typed StrategyError and warn! no-op for optimization. |
| src/strategies/long_put.rs | Replace todo!() stubs with typed StrategyError and warn! no-op for optimization. |
| src/strategies/long_call.rs | Replace todo!() stubs with typed StrategyError, add a negative test for get_strategy. |
| src/strategies/base.rs | Replace trait-default panics with warnings/empty defaults or typed Err(...) for unsupported operations; keep explicit panics for reference-return defaults. |
| src/model/position.rs | Replace TransactionAble todo!() with TransactionError returns and add negative tests. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Codecov Report❌ Patch coverage is
... and 1 file with indirect coverage changes 🚀 New features to boost your workflow:
|
…n test, reword no-op doc - src/strategies/base.rs: replace literal "default" strategy reason in 11 trait-default error sites (set_expiration_date, set_underlying_price, set_implied_volatility, roll_in, roll_out, update_break_even_points, get_position, get_position_unique, get_option_unique, modify_position, replace_position) with std::any::type_name::<Self>(). Matches the pattern already used by add_position / get_positions and produces actionable error messages that name the actual implementing strategy. - src/strategies/base.rs: get_title, get_option_basic_type, get_implied_volatility, get_quantity tracing::warn! messages now include std::any::type_name::<Self>() so a missing override is attributable to a concrete type from the log. - src/strategies/long_call.rs: tighten the get_strategy negative test to assert OperationErrorKind::NotSupported with operation "get_strategy" and reason containing "LongCall", instead of just matching the outer OperationError variant. - src/model/position.rs: reword the TransactionAble docstring from "Default no-op stub" to "intentionally unsupported" since the impl always errors rather than no-oping.
Owner
Author
|
Thanks for the review. All six findings are addressed in f28cb15. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Removes every
todo!()andunimplemented!()from production code perrules/global_rules.md§DO NOT. 27 sites eliminated across 6 files:src/strategies/base.rs— 17 trait defaultssrc/strategies/{long,short}_{call,put}.rs— 8 stubs (4 files × 2)src/model/position.rs— 2TransactionAblestubsAfter this PR the acceptance grep returns 0 matches across all
src/**/*.rs:Per-pattern transformations
unimplemented!(...)inResult<_, StrategyError>fnErr(StrategyError::operation_not_supported(<op>, "default"))unimplemented!(...)inResult<_, PositionError>fnErr(PositionError::unsupported_operation("default", <op>))todo!()inResult<Self, StrategyError>::get_strategyErr(StrategyError::operation_not_supported("get_strategy", "<Strategy>"))todo!()inResult<_, TransactionError>fnErr(TransactionError { message: "<X> not implemented for Position".into() })todo!()in()-returningfind_optimaloverridetracing::warn!(...)no-opunimplemented!(...)in non-Result getter (String,HashSet,HashMap)String::new()default +tracing::warn!unimplemented!(...)returning&Options/&mut Optionspanic!("...")+# Panicsdoc (no graceful fallback)Public-surface notes
Err(...)or no-op +tracing::warn!. Callers that pattern-matchErrcontinue to work; callers that relied on the panic see graceful failure.one_option/one_option_mutkeeppanic!because&Options/&mut Optionscan't be defaulted; switching toOption<&Options>would touch every caller — tracked as a follow-up.prelude.rschange. No semver-relevant signature change.Test plan
src/**/*.rscargo clippy --all-targets --all-features --workspace -- -D warningscargo fmt --all --checkcargo test --all-features --workspacecargo build --releasemake pre-push(fix + fmt + lint-fix + test + readme + doc)LongCall::get_strategy,Position::add_transaction,Position::get_transactions)Commit layout
3 atomic per-bucket commits:
89a9790c— base.rs trait defaults (17 sites)5061a547— strategy stubs (8 sites + 1 negative test)01f6b88c— Position TransactionAble (2 sites + 2 negative tests)Closes #322