From 89a9790cbc8815ec8d0ba728f3fddd08d29cdfd8 Mon Sep 17 00:00:00 2001 From: Joaquin Bejar Date: Sat, 18 Apr 2026 05:39:38 +0200 Subject: [PATCH 1/4] refactor(strategies): drop unimplemented! from base trait defaults (#322) 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. --- src/strategies/base.rs | 308 ++++++++++++++++++++--------------------- 1 file changed, 149 insertions(+), 159 deletions(-) diff --git a/src/strategies/base.rs b/src/strategies/base.rs index 4607f8f4..53701e94 100644 --- a/src/strategies/base.rs +++ b/src/strategies/base.rs @@ -27,7 +27,7 @@ use serde::{Deserialize, Serialize}; use std::collections::{HashMap, HashSet}; use std::fmt; use std::str::FromStr; -use tracing::error; +use tracing::{error, warn}; use utoipa::ToSchema; /// Represents basic information about a trading strategy. @@ -353,8 +353,9 @@ impl Strategy { /// expiration dates, and implied volatility. /// /// # Note -/// Several methods in this trait are unimplemented and will panic if invoked -/// without being explicitly implemented for the specific strategy. +/// Most defaults return either an empty value (with a `tracing::warn!` log) +/// or an `Err(...)` describing the unsupported operation. Concrete strategies +/// should override the methods they need. /// /// # Methods /// - `get_title`: Returns the title of the strategy. @@ -366,39 +367,41 @@ impl Strategy { /// - `get_type`: Retrieves the type of the option. /// - `get_style`: Maps option basic types to their corresponding styles. /// - `get_expiration`: Maps option basic types to their expiration dates. -/// - `get_implied_volatility`: Retrieves implied volatility (currently unimplemented). +/// - `get_implied_volatility`: Retrieves implied volatility. /// /// # Panics -/// All methods with `unimplemented!` will panic when called unless properly implemented. -/// Refer to the documentation for individual methods for more details. +/// Only `one_option` and `one_option_mut` panic on the default implementation, +/// because their reference return types do not allow a graceful fallback. +/// Every strategy that owns `Options` must override both methods. /// pub trait BasicAble { /// Retrieves the title associated with the current instance of the strategy. /// /// # Returns - /// A `String` representing the title. + /// A `String` representing the title. /// - /// # Panics - /// This method is not yet implemented and will panic with the message - /// `"get_title is not implemented for this strategy"`. Ensure this method - /// is properly implemented before using it. + /// # Default + /// The default implementation returns an empty `String` and emits a + /// `tracing::warn!` log so callers can detect strategies that did not + /// override this method. /// fn get_title(&self) -> String { - unimplemented!("get_title is not implemented for this strategy"); + warn!("get_title default: strategy did not override; returning empty string"); + String::new() } /// Retrieves a `HashSet` of `OptionBasicType` values associated with the current strategy. /// /// # Returns /// A `HashSet` containing the `OptionBasicType` elements relevant to the strategy. - /// However, this method is currently not implemented and will panic with the message - /// `"get_option_basic_type is not implemented for this strategy"`. /// - /// # Panics - /// This function will panic with the message: - /// `"get_option_basic_type is not implemented for this strategy"` if called. + /// # Default + /// The default implementation returns an empty `HashSet` and emits a + /// `tracing::warn!` log so callers can detect strategies that did not + /// override this method. /// fn get_option_basic_type(&self) -> HashSet> { - unimplemented!("get_option_basic_type is not implemented for this strategy"); + warn!("get_option_basic_type default: strategy did not override; returning empty set"); + HashSet::new() } /// Retrieves the symbol associated with the current instance by delegating the call to the `get_symbol` /// method of the `one_option` object. @@ -549,17 +552,14 @@ pub trait BasicAble { /// pair corresponds to the implied volatility associated with a /// specific option type. /// - /// # Notes + /// # Default /// - /// This method is not yet implemented for the specific strategy - /// and will panic when invoked. - /// - /// # Panics - /// - /// This function will unconditionally panic with the message - /// `"get_implied_volatility is not implemented for this strategy"`. + /// The default implementation returns an empty `HashMap` and emits a + /// `tracing::warn!` log so callers can detect strategies that did not + /// override this method. fn get_implied_volatility(&self) -> HashMap, &Positive> { - unimplemented!("get_implied_volatility is not implemented for this strategy"); + warn!("get_implied_volatility default: strategy did not override; returning empty map"); + HashMap::new() } /// Retrieves the quantity information associated with the strategy. /// @@ -568,18 +568,17 @@ pub trait BasicAble { /// to a `Positive` value (the value). This map represents the mapping of /// option basic types to their respective quantities. /// - /// # Notes - /// This method is not implemented in the current strategy and will - /// panic when called. - /// - /// # Panics - /// This function will panic with the message `"get_quantity is not implemented for this strategy"`. + /// # Default + /// The default implementation returns an empty `HashMap` and emits a + /// `tracing::warn!` log so callers can detect strategies that did not + /// override this method. /// /// # Example /// The function currently serves as a placeholder and should be implemented /// in a specific strategy that defines its behavior. fn get_quantity(&self) -> HashMap, &Positive> { - unimplemented!("get_quantity is not implemented for this strategy"); + warn!("get_quantity default: strategy did not override; returning empty map"); + HashMap::new() } /// Retrieves the underlying price of the financial instrument (e.g., option). /// @@ -637,84 +636,68 @@ pub trait BasicAble { fn get_dividend_yield(&self) -> HashMap, &Positive> { self.one_option().get_dividend_yield() } - /// This method, `one_option`, is designed to retrieve a reference to an `Options` object. - /// However, in this implementation, the function is not currently functional, as it - /// explicitly triggers an unimplemented error when called. + /// Retrieves a shared reference to the strategy's primary `Options` value. /// /// # Returns - /// * `&Options` - A reference to an `Options` object. However, this is not currently - /// available due to the method being unimplemented. + /// * `&Options` - A reference to an `Options` object owned by the strategy. /// /// # Panics - /// This method will unconditionally panic with the message - /// "one_option is not implemented for this strategy" whenever it is invoked. + /// The default implementation panics because there is no graceful fallback + /// for a borrowed `&Options` return. Every strategy that owns options + /// must override this method. /// /// # Note - /// This is a placeholder implementation and should be overridden or implemented in - /// a concrete type where this function is required. + /// This is a placeholder implementation and must be overridden in any + /// concrete strategy that holds option positions. fn one_option(&self) -> &Options { - unimplemented!("one_option is not implemented for this strategy"); + panic!( + "one_option not implemented for this strategy — every strategy with options must override" + ) } - /// Provides a mutable reference to an `Options` instance. + /// Provides a mutable reference to the strategy's primary `Options` value. /// - /// This function is intended to allow mutation of a single - /// `Options` instance managed within the strategy. It is - /// a stub and not currently implemented. When called, - /// it will panic with the message "one_option_mut is not implemented - /// for this strategy". - /// - /// # Errors + /// # Panics /// - /// Panics if this function is called since it is unimplemented. + /// The default implementation panics because there is no graceful fallback + /// for a borrowed `&mut Options` return. Every strategy that owns options + /// must override this method. /// /// # Returns /// - /// A mutable reference to an `Options` instance (in a fully - /// implemented version of this function). + /// A mutable reference to an `Options` instance. /// fn one_option_mut(&mut self) -> &mut Options { - unimplemented!("one_option_mut is not implemented for this strategy"); + panic!( + "one_option_mut not implemented for this strategy — every strategy with options must override" + ) } /// Sets the expiration date for the strategy. /// - /// This method is intended to allow the user to define an expiration date - /// for a given strategy. However, it is currently unimplemented for this - /// specific strategy and will result in a panic with a message indicating - /// that it is not supported. - /// /// # Parameters /// /// - `_expiration_date`: The expiration date to set for the strategy, - /// represented as an `ExpirationDate` object. This parameter is accepted - /// but not utilized, as the method is not implemented. + /// represented as an `ExpirationDate` object. /// /// # Returns /// - /// This function returns a `Result`: - /// - `Ok(())` if the operation is successful (not applicable here as the - /// function is unimplemented). - /// - `Err(StrategyError)` if an error occurs (though, in this case, the - /// method only panics as it is unimplemented). + /// - `Ok(())` if the operation is successful. + /// - `Err(StrategyError)` if the strategy does not support setting + /// expiration dates. /// /// # Errors /// - /// Always returns a panic with the message: - /// `"set_expiration_date is not implemented for this strategy"`. No actual - /// `StrategyError` is produced by this method, as it is incomplete. - /// - /// # Panics - /// - /// This function always panics when called with the message: - /// `"set_expiration_date is not implemented for this strategy"`. - /// - /// Note: Avoid using this method until it is fully implemented for this - /// specific strategy. + /// The default implementation returns + /// `StrategyError::OperationError(NotSupported { .. })`. Strategies that + /// support mutating expiration should override this method. fn set_expiration_date( &mut self, _expiration_date: ExpirationDate, ) -> Result<(), StrategyError> { - unimplemented!("set_expiration_date is not implemented for this strategy"); + Err(StrategyError::operation_not_supported( + "set_expiration_date", + "default", + )) } /// Sets the underlying price for this strategy. /// @@ -724,18 +707,19 @@ pub trait BasicAble { /// /// # Returns /// - `Ok(())` if the operation is successful. - /// - `Err(StrategyError)` if an error occurs during the operation. - /// - /// # Note - /// This function is currently not implemented for this strategy. Calling - /// this function will result in a runtime panic with the message - /// "set_underlying_price is not implemented for this strategy". + /// - `Err(StrategyError)` if the strategy does not support setting the + /// underlying price. /// - /// # Panics - /// Always panics with the message `"set_underlying_price is not implemented for this strategy"`. + /// # Errors + /// The default implementation returns + /// `StrategyError::OperationError(NotSupported { .. })`. Strategies that + /// support mutating the underlying price should override this method. /// fn set_underlying_price(&mut self, _price: &Positive) -> Result<(), StrategyError> { - unimplemented!("set_underlying_price is not implemented for this strategy"); + Err(StrategyError::operation_not_supported( + "set_underlying_price", + "default", + )) } /// Updates the volatility for the strategy. /// @@ -743,15 +727,20 @@ pub trait BasicAble { /// - `_volatility`: A reference to a `Positive` value representing the new volatility to set. /// /// # Returns - /// - `Ok(())`: If the update operation succeeds (currently unimplemented). - /// - `Err(StrategyError)`: If there is an error during the update process (place-holder as functionality is not implemented). + /// - `Ok(())`: If the update operation succeeds. + /// - `Err(StrategyError)`: If the strategy does not support setting the + /// implied volatility. /// - /// # Notes - /// This method is currently unimplemented, and calling it will result in the `unimplemented!` macro being triggered, which causes a panic. - /// This function is a stub and should be implemented to handle setting the volatility specific to the strategy. + /// # Errors + /// The default implementation returns + /// `StrategyError::OperationError(NotSupported { .. })`. Strategies that + /// support mutating implied volatility should override this method. /// fn set_implied_volatility(&mut self, _volatility: &Positive) -> Result<(), StrategyError> { - unimplemented!("set_implied_volatility is not implemented for this strategy"); + Err(StrategyError::operation_not_supported( + "set_implied_volatility", + "default", + )) } } @@ -1063,34 +1052,24 @@ pub trait Strategies: Validable + Positionable + BreakEvenable + BasicAble { /// # Parameters /// - `&mut self`: A mutable reference to the current instance of the strategy. /// - `_position: &Position`: A reference to the `Position` object, representing the current position - /// in the market. This parameter is currently unused in the implementation. + /// in the market. This parameter is currently unused by the default implementation. /// /// # Returns /// - `Result, StrategyError>`: /// - `Ok(HashMap)`: On success, a map of actions to trades, representing the changes /// made during the roll-in process. - /// - `Err(StrategyError)`: If an error occurs during the roll-in operation. + /// - `Err(StrategyError)`: If the strategy does not support rolling in. /// /// # Errors - /// - Returns a `StrategyError` if the roll-in operation fails (not currently implemented). - /// - /// # Panics - /// - This function will panic if called, as it is currently unimplemented. - /// - /// # Note - /// - This method is not implemented and will panic upon invocation. Future implementations should - /// define the specific logic for handling the roll-in operation for the associated strategy. + /// The default implementation returns + /// `StrategyError::OperationError(NotSupported { .. })`. Strategies that + /// support roll-in should override this method. fn roll_in(&mut self, _position: &Position) -> Result, StrategyError> { - unimplemented!("roll_in is not implemented for this strategy") + Err(StrategyError::operation_not_supported("roll_in", "default")) } /// Executes the roll-out strategy for the provided position. /// - /// This function is intended to evaluate and execute trading actions based - /// on the given `Position`. It returns a mapping of `Action` to `Trade` that - /// represents the proposed trades resulting from the strategy. However, this - /// method currently is not implemented and will panic if called. - /// /// # Arguments /// /// * `_position` - A reference to a `Position` object which represents the @@ -1100,26 +1079,18 @@ pub trait Strategies: Validable + Positionable + BreakEvenable + BasicAble { /// /// * `Result, StrategyError>` - A `Result` object /// containing: - /// - `Ok(HashMap)` with the mapping of actions to trades if - /// successfully implemented in the future. - /// - `Err(StrategyError)` if an error occurs during execution (currently - /// always unimplemented). + /// - `Ok(HashMap)` with the mapping of actions to trades. + /// - `Err(StrategyError)` if the strategy does not support rolling out. /// /// # Errors /// - /// * Returns an error of type `StrategyError` if the strategy encounters - /// execution issues (in this case, always unimplemented). - /// - /// # Panics - /// - /// This function will panic with a message "roll_out is not implemented for this - /// strategy" since it is currently not implemented. - /// - /// # Note - /// - /// Until implemented, calling this method will result in a runtime panic. + /// The default implementation returns + /// `StrategyError::OperationError(NotSupported { .. })`. Strategies that + /// support roll-out should override this method. fn roll_out(&mut self, _position: &Position) -> Result, StrategyError> { - unimplemented!("roll_out is not implemented for this strategy") + Err(StrategyError::operation_not_supported( + "roll_out", "default", + )) } } @@ -1147,10 +1118,16 @@ pub trait BreakEvenable { /// This method is responsible for recalculating and updating the break-even points based on /// the current state of the strategy. /// - /// The default implementation returns a `NotImplemented` error. Strategies implementing this trait - /// should override this method to provide specific update logic. + /// # Errors + /// The default implementation returns + /// `StrategyError::OperationError(NotSupported { .. })`. Strategies + /// implementing this trait should override this method to provide + /// specific update logic. fn update_break_even_points(&mut self) -> Result<(), StrategyError> { - unimplemented!("Update break even points is not implemented for this strategy") + Err(StrategyError::operation_not_supported( + "update_break_even_points", + "default", + )) } } @@ -1374,14 +1351,21 @@ pub trait Positionable { /// /// * `Result, PositionError>` - A `Result` containing a vector of mutable /// references to the matching `Position` objects, or a `PositionError` if the operation is not supported. - /// This function currently uses `unimplemented!()`. + /// + /// # Errors + /// The default implementation returns + /// `PositionError::unsupported_operation(..)`. Strategies that manage + /// positions should override this method. fn get_position( &mut self, _option_style: &OptionStyle, _side: &Side, _strike: &Positive, ) -> Result, PositionError> { - unimplemented!("Modify position is not implemented for this strategy") + Err(PositionError::unsupported_operation( + "default", + "get_position", + )) } /// Retrieves a unique position based on the given option style and side. @@ -1395,24 +1379,23 @@ pub trait Positionable { /// returns a `PositionError`. /// /// # Errors - /// This function always returns an error as it is not implemented for this strategy but provides a placeholder - /// for functionality to be added later. + /// The default implementation returns + /// `PositionError::unsupported_operation(..)`. Strategies that expose a + /// unique position per (style, side) pair should override this method. /// fn get_position_unique( &mut self, _option_style: &OptionStyle, _side: &Side, ) -> Result<&mut Position, PositionError> { - unimplemented!("Get unique position is not implemented for this strategy") + Err(PositionError::unsupported_operation( + "default", + "get_position_unique", + )) } /// Retrieves a unique option based on the given style and side. /// - /// This function is intended to retrieve a unique financial option of a specific - /// style (`_option_style`) and side (`_side`). However, the functionality has - /// not been implemented for the current strategy, and calling this function - /// will result in a runtime panic. - /// /// # Parameters /// - `_option_style`: A reference to an `OptionStyle` that specifies the style /// of the option to retrieve (e.g., American, European). @@ -1421,25 +1404,23 @@ pub trait Positionable { /// /// # Returns /// - `Result<&mut Options, PositionError>`: - /// - On success, a mutable reference to an `Options` object would be returned. - /// However, the current implementation always results in an unimplemented - /// error. + /// - On success, a mutable reference to an `Options` object. + /// - On failure, a `PositionError`. /// /// # Errors - /// - Always returns a `PositionError` due to the `unimplemented!` macro indicating - /// that this functionality is not yet supported for the strategy. - /// - /// # Notes - /// This function should be implemented to support strategies that require - /// retrieving unique options. Until implemented, usage of this function - /// is not recommended. + /// The default implementation returns + /// `PositionError::unsupported_operation(..)`. Strategies that expose a + /// unique option per (style, side) pair should override this method. /// fn get_option_unique( &mut self, _option_style: &OptionStyle, _side: &Side, ) -> Result<&mut Options, PositionError> { - unimplemented!("Get unique option is not implemented for this strategy") + Err(PositionError::unsupported_operation( + "default", + "get_option_unique", + )) } /// Modifies an existing position. @@ -1452,9 +1433,16 @@ pub trait Positionable { /// /// * `Result<(), PositionError>` - A `Result` indicating success or failure of the /// modification, or a `PositionError` if the operation is not supported. - /// This function currently uses `unimplemented!()`. + /// + /// # Errors + /// The default implementation returns + /// `PositionError::unsupported_operation(..)`. Strategies that allow + /// in-place modification of positions should override this method. fn modify_position(&mut self, _position: &Position) -> Result<(), PositionError> { - unimplemented!("Modify position is not implemented for this strategy") + Err(PositionError::unsupported_operation( + "default", + "modify_position", + )) } /// @@ -1467,14 +1455,16 @@ pub trait Positionable { /// - `Ok(())`: If the position replacement is successful. /// - `Err(PositionError)`: If an error occurs while replacing the position. /// - /// # Notes - /// This function is currently not implemented for this strategy and will panic with a `not implemented` message when called. - /// - /// # Panics - /// This function will always panic with `unimplemented!()` since it hasn't been implemented yet. + /// # Errors + /// The default implementation returns + /// `PositionError::unsupported_operation(..)`. Strategies that allow + /// replacing positions should override this method. /// fn replace_position(&mut self, _position: &Position) -> Result<(), PositionError> { - unimplemented!("Replace position is not implemented for this strategy") + Err(PositionError::unsupported_operation( + "default", + "replace_position", + )) } /// Checks if all short positions have a net premium received that meets or exceeds a specified minimum. From 5061a547f009560c29379f871b1c5f219339103f Mon Sep 17 00:00:00 2001 From: Joaquin Bejar Date: Sat, 18 Apr 2026 05:39:43 +0200 Subject: [PATCH 2/4] refactor(strategies): drop todo!() from long/short call/put stubs (#322) Each of the four single-leg strategies had two `todo!()` stubs: - `StrategyConstructor::get_strategy(&[Position]) -> Result` now returns `Err(StrategyError::operation_not_supported("get_strategy", ""))` 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. --- src/strategies/long_call.rs | 20 +++++++++++++++++--- src/strategies/long_put.rs | 9 ++++++--- src/strategies/short_call.rs | 9 ++++++--- src/strategies/short_put.rs | 9 ++++++--- 4 files changed, 35 insertions(+), 12 deletions(-) diff --git a/src/strategies/long_call.rs b/src/strategies/long_call.rs index 122e2808..241d4d70 100644 --- a/src/strategies/long_call.rs +++ b/src/strategies/long_call.rs @@ -33,7 +33,7 @@ use pretty_simple_display::{DebugPretty, DisplaySimple}; use rust_decimal::Decimal; use serde::{Deserialize, Serialize}; use std::collections::{HashMap, HashSet}; -use tracing::debug; +use tracing::{debug, warn}; use utoipa::ToSchema; pub(super) const LONG_CALL_DESCRIPTION: &str = "A Long Call is an options strategy where the trader buys a call option, acquiring the right (but not the obligation) to purchase the underlying asset at the strike price until expiration. \ @@ -396,7 +396,10 @@ impl Positionable for LongCall { impl StrategyConstructor for LongCall { fn get_strategy(_vec_positions: &[Position]) -> Result { - todo!() + Err(StrategyError::operation_not_supported( + "get_strategy", + "LongCall", + )) } } @@ -409,7 +412,7 @@ impl Optimizable for LongCall { _side: crate::strategies::FindOptimalSide, _criteria: OptimizationCriteria, ) { - todo!() + warn!("find_optimal: stub — no optimization performed for LongCall"); } } @@ -755,6 +758,17 @@ where impl Strategable for LongCall {} +#[cfg(test)] +mod tests_get_strategy { + use super::*; + + #[test] + fn test_get_strategy_returns_not_supported() { + let result = LongCall::get_strategy(&[]); + assert!(matches!(result, Err(StrategyError::OperationError(_)))); + } +} + #[cfg(test)] mod tests_simulate { use super::*; diff --git a/src/strategies/long_put.rs b/src/strategies/long_put.rs index ef9e036b..0cd76849 100644 --- a/src/strategies/long_put.rs +++ b/src/strategies/long_put.rs @@ -36,7 +36,7 @@ use pretty_simple_display::{DebugPretty, DisplaySimple}; use rust_decimal::Decimal; use serde::{Deserialize, Serialize}; use std::collections::{HashMap, HashSet}; -use tracing::debug; +use tracing::{debug, warn}; use utoipa::ToSchema; pub(super) const LONG_PUT_DESCRIPTION: &str = "A Long Put is an options strategy where the trader purchases a put option, gaining the right (but not the obligation) to sell the underlying asset at the strike price until expiration. \ @@ -403,7 +403,10 @@ impl Positionable for LongPut { impl StrategyConstructor for LongPut { fn get_strategy(_vec_positions: &[Position]) -> Result { - todo!() + Err(StrategyError::operation_not_supported( + "get_strategy", + "LongPut", + )) } } @@ -416,7 +419,7 @@ impl Optimizable for LongPut { _side: FindOptimalSide, _criteria: OptimizationCriteria, ) { - todo!() + warn!("find_optimal: stub — no optimization performed for LongPut"); } } diff --git a/src/strategies/short_call.rs b/src/strategies/short_call.rs index c28e6158..7294a2ec 100644 --- a/src/strategies/short_call.rs +++ b/src/strategies/short_call.rs @@ -35,7 +35,7 @@ use pretty_simple_display::{DebugPretty, DisplaySimple}; use rust_decimal::Decimal; use serde::{Deserialize, Serialize}; use std::collections::{HashMap, HashSet}; -use tracing::debug; +use tracing::{debug, warn}; use utoipa::ToSchema; pub(super) const SHORT_CALL_DESCRIPTION: &str = "A Short Call (or Naked Call) is an options strategy where the trader sells a call option without owning the underlying stock. \ @@ -407,7 +407,10 @@ impl Positionable for ShortCall { impl StrategyConstructor for ShortCall { fn get_strategy(_vec_positions: &[Position]) -> Result { - todo!() + Err(StrategyError::operation_not_supported( + "get_strategy", + "ShortCall", + )) } } @@ -420,7 +423,7 @@ impl Optimizable for ShortCall { _side: FindOptimalSide, _criteria: OptimizationCriteria, ) { - todo!() + warn!("find_optimal: stub — no optimization performed for ShortCall"); } } diff --git a/src/strategies/short_put.rs b/src/strategies/short_put.rs index 10b2c597..bd6d86a8 100644 --- a/src/strategies/short_put.rs +++ b/src/strategies/short_put.rs @@ -36,7 +36,7 @@ use pretty_simple_display::{DebugPretty, DisplaySimple}; use rust_decimal::Decimal; use serde::{Deserialize, Serialize}; use std::collections::{HashMap, HashSet}; -use tracing::debug; +use tracing::{debug, warn}; use utoipa::ToSchema; pub(super) const SHORT_PUT_DESCRIPTION: &str = "A Short Put (or Naked Put) is an options strategy where the trader sells a put option without holding a short position in the underlying stock. \ @@ -408,7 +408,10 @@ impl Positionable for ShortPut { impl StrategyConstructor for ShortPut { fn get_strategy(_vec_positions: &[Position]) -> Result { - todo!() + Err(StrategyError::operation_not_supported( + "get_strategy", + "ShortPut", + )) } } @@ -421,7 +424,7 @@ impl Optimizable for ShortPut { _side: FindOptimalSide, _criteria: OptimizationCriteria, ) { - todo!() + warn!("find_optimal: stub — no optimization performed for ShortPut"); } } From 01f6b88c44f2901fa4c91818093a90d2619107e1 Mon Sep 17 00:00:00 2001 From: Joaquin Bejar Date: Sat, 18 Apr 2026 05:39:46 +0200 Subject: [PATCH 3/4] refactor(position): drop todo!() from TransactionAble stubs (#322) `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. --- src/model/position.rs | 85 ++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 83 insertions(+), 2 deletions(-) diff --git a/src/model/position.rs b/src/model/position.rs index a592aa63..a0bb136e 100644 --- a/src/model/position.rs +++ b/src/model/position.rs @@ -746,12 +746,93 @@ impl Greeks for Position { } impl TransactionAble for Position { + /// Default no-op stub. `Position` does not currently track an + /// internal transaction history; callers that need this should + /// store transactions in a higher-level container. + /// + /// # Errors + /// + /// Always returns a `TransactionError` indicating the operation is + /// not implemented for `Position`. fn add_transaction(&mut self, _transaction: Transaction) -> Result<(), TransactionError> { - todo!() + Err(TransactionError { + message: "add_transaction not implemented for Position".to_string(), + }) } + /// Default no-op stub. See [`Self::add_transaction`]. + /// + /// # Errors + /// + /// Always returns a `TransactionError` indicating the operation is + /// not implemented for `Position`. fn get_transactions(&self) -> Result, TransactionError> { - todo!() + Err(TransactionError { + message: "get_transactions not implemented for Position".to_string(), + }) + } +} + +#[cfg(test)] +mod tests_transaction_able_default { + use super::*; + use crate::model::TradeStatus; + use crate::model::types::{OptionStyle, OptionType, Side}; + use crate::model::utils::create_sample_option_simplest; + use chrono::Utc; + use positive::pos_or_panic; + + fn make_position() -> Position { + let option = create_sample_option_simplest(OptionStyle::Call, Side::Long); + Position::new( + option, + pos_or_panic!(5.25), + Utc::now(), + pos_or_panic!(0.65), + pos_or_panic!(0.65), + None, + None, + ) + } + + fn make_transaction() -> Transaction { + Transaction::new( + TradeStatus::Open, + None, + OptionType::European, + Side::Long, + OptionStyle::Call, + pos_or_panic!(1.0), + pos_or_panic!(5.25), + pos_or_panic!(0.65), + None, + None, + None, + ) + } + + #[test] + fn test_add_transaction_returns_not_implemented() { + let mut position = make_position(); + let result = position.add_transaction(make_transaction()); + match result { + Err(TransactionError { message }) => { + assert!(message.contains("add_transaction not implemented")); + } + Ok(()) => panic!("expected TransactionError, got Ok"), + } + } + + #[test] + fn test_get_transactions_returns_not_implemented() { + let position = make_position(); + let result = position.get_transactions(); + match result { + Err(TransactionError { message }) => { + assert!(message.contains("get_transactions not implemented")); + } + Ok(_) => panic!("expected TransactionError, got Ok"), + } } } From f28cb15ee99afec035848de64fcadd5669b1c2a5 Mon Sep 17 00:00:00 2001 From: Joaquin Bejar Date: Sat, 18 Apr 2026 05:54:38 +0200 Subject: [PATCH 4/4] address review: type_name in default error reasons + warnings, tighten 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::(). 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::() 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. --- src/model/position.rs | 18 ++++++++------- src/strategies/base.rs | 46 +++++++++++++++++++++++++------------ src/strategies/long_call.rs | 15 +++++++++++- 3 files changed, 55 insertions(+), 24 deletions(-) diff --git a/src/model/position.rs b/src/model/position.rs index a0bb136e..45dbfbb9 100644 --- a/src/model/position.rs +++ b/src/model/position.rs @@ -746,26 +746,28 @@ impl Greeks for Position { } impl TransactionAble for Position { - /// Default no-op stub. `Position` does not currently track an - /// internal transaction history; callers that need this should - /// store transactions in a higher-level container. + /// `Position` does not track an internal transaction history. + /// This impl is intentionally unsupported; callers that need + /// transaction tracking should store transactions in a + /// higher-level container. /// /// # Errors /// - /// Always returns a `TransactionError` indicating the operation is - /// not implemented for `Position`. + /// Always returns a `TransactionError` — this operation is + /// intentionally unsupported on `Position`. fn add_transaction(&mut self, _transaction: Transaction) -> Result<(), TransactionError> { Err(TransactionError { message: "add_transaction not implemented for Position".to_string(), }) } - /// Default no-op stub. See [`Self::add_transaction`]. + /// See [`Self::add_transaction`] — this method is intentionally + /// unsupported on `Position` and always errors. /// /// # Errors /// - /// Always returns a `TransactionError` indicating the operation is - /// not implemented for `Position`. + /// Always returns a `TransactionError` — this operation is + /// intentionally unsupported on `Position`. fn get_transactions(&self) -> Result, TransactionError> { Err(TransactionError { message: "get_transactions not implemented for Position".to_string(), diff --git a/src/strategies/base.rs b/src/strategies/base.rs index 53701e94..0bef5bd7 100644 --- a/src/strategies/base.rs +++ b/src/strategies/base.rs @@ -386,7 +386,10 @@ pub trait BasicAble { /// override this method. /// fn get_title(&self) -> String { - warn!("get_title default: strategy did not override; returning empty string"); + warn!( + "get_title default: {} did not override; returning empty string", + std::any::type_name::() + ); String::new() } /// Retrieves a `HashSet` of `OptionBasicType` values associated with the current strategy. @@ -400,7 +403,10 @@ pub trait BasicAble { /// override this method. /// fn get_option_basic_type(&self) -> HashSet> { - warn!("get_option_basic_type default: strategy did not override; returning empty set"); + warn!( + "get_option_basic_type default: {} did not override; returning empty set", + std::any::type_name::() + ); HashSet::new() } /// Retrieves the symbol associated with the current instance by delegating the call to the `get_symbol` @@ -558,7 +564,10 @@ pub trait BasicAble { /// `tracing::warn!` log so callers can detect strategies that did not /// override this method. fn get_implied_volatility(&self) -> HashMap, &Positive> { - warn!("get_implied_volatility default: strategy did not override; returning empty map"); + warn!( + "get_implied_volatility default: {} did not override; returning empty map", + std::any::type_name::() + ); HashMap::new() } /// Retrieves the quantity information associated with the strategy. @@ -577,7 +586,10 @@ pub trait BasicAble { /// The function currently serves as a placeholder and should be implemented /// in a specific strategy that defines its behavior. fn get_quantity(&self) -> HashMap, &Positive> { - warn!("get_quantity default: strategy did not override; returning empty map"); + warn!( + "get_quantity default: {} did not override; returning empty map", + std::any::type_name::() + ); HashMap::new() } /// Retrieves the underlying price of the financial instrument (e.g., option). @@ -696,7 +708,7 @@ pub trait BasicAble { ) -> Result<(), StrategyError> { Err(StrategyError::operation_not_supported( "set_expiration_date", - "default", + std::any::type_name::(), )) } /// Sets the underlying price for this strategy. @@ -718,7 +730,7 @@ pub trait BasicAble { fn set_underlying_price(&mut self, _price: &Positive) -> Result<(), StrategyError> { Err(StrategyError::operation_not_supported( "set_underlying_price", - "default", + std::any::type_name::(), )) } /// Updates the volatility for the strategy. @@ -739,7 +751,7 @@ pub trait BasicAble { fn set_implied_volatility(&mut self, _volatility: &Positive) -> Result<(), StrategyError> { Err(StrategyError::operation_not_supported( "set_implied_volatility", - "default", + std::any::type_name::(), )) } } @@ -1065,7 +1077,10 @@ pub trait Strategies: Validable + Positionable + BreakEvenable + BasicAble { /// `StrategyError::OperationError(NotSupported { .. })`. Strategies that /// support roll-in should override this method. fn roll_in(&mut self, _position: &Position) -> Result, StrategyError> { - Err(StrategyError::operation_not_supported("roll_in", "default")) + Err(StrategyError::operation_not_supported( + "roll_in", + std::any::type_name::(), + )) } /// Executes the roll-out strategy for the provided position. @@ -1089,7 +1104,8 @@ pub trait Strategies: Validable + Positionable + BreakEvenable + BasicAble { /// support roll-out should override this method. fn roll_out(&mut self, _position: &Position) -> Result, StrategyError> { Err(StrategyError::operation_not_supported( - "roll_out", "default", + "roll_out", + std::any::type_name::(), )) } } @@ -1126,7 +1142,7 @@ pub trait BreakEvenable { fn update_break_even_points(&mut self) -> Result<(), StrategyError> { Err(StrategyError::operation_not_supported( "update_break_even_points", - "default", + std::any::type_name::(), )) } } @@ -1363,7 +1379,7 @@ pub trait Positionable { _strike: &Positive, ) -> Result, PositionError> { Err(PositionError::unsupported_operation( - "default", + std::any::type_name::(), "get_position", )) } @@ -1389,7 +1405,7 @@ pub trait Positionable { _side: &Side, ) -> Result<&mut Position, PositionError> { Err(PositionError::unsupported_operation( - "default", + std::any::type_name::(), "get_position_unique", )) } @@ -1418,7 +1434,7 @@ pub trait Positionable { _side: &Side, ) -> Result<&mut Options, PositionError> { Err(PositionError::unsupported_operation( - "default", + std::any::type_name::(), "get_option_unique", )) } @@ -1440,7 +1456,7 @@ pub trait Positionable { /// in-place modification of positions should override this method. fn modify_position(&mut self, _position: &Position) -> Result<(), PositionError> { Err(PositionError::unsupported_operation( - "default", + std::any::type_name::(), "modify_position", )) } @@ -1462,7 +1478,7 @@ pub trait Positionable { /// fn replace_position(&mut self, _position: &Position) -> Result<(), PositionError> { Err(PositionError::unsupported_operation( - "default", + std::any::type_name::(), "replace_position", )) } diff --git a/src/strategies/long_call.rs b/src/strategies/long_call.rs index 241d4d70..a0ed9ec2 100644 --- a/src/strategies/long_call.rs +++ b/src/strategies/long_call.rs @@ -764,8 +764,21 @@ mod tests_get_strategy { #[test] fn test_get_strategy_returns_not_supported() { + use crate::prelude::OperationErrorKind; let result = LongCall::get_strategy(&[]); - assert!(matches!(result, Err(StrategyError::OperationError(_)))); + match result { + Err(StrategyError::OperationError(OperationErrorKind::NotSupported { + operation, + reason, + })) => { + assert_eq!(operation, "get_strategy"); + assert!( + reason.contains("LongCall"), + "expected reason to contain 'LongCall', got {reason}" + ); + } + other => panic!("expected NotSupported error, got {other:?}"), + } } }