ChrisCScott / forecaster

A personal finances forecasting tool for Canadian retirement planning
Other
1 stars 2 forks source link

Handle transactions to/from multiple accounts of the same type #4

Closed ChrisCScott closed 6 years ago

ChrisCScott commented 6 years ago

TransactionStrategy determines withdrawals based on account type (e.g. ordered, weighted, etc.). At present, behaviour when multiple accounts of the same type are passed is undefined. We need to define it.

Ideally, treat them as one account and split transactions between them in a reasonable way. Consider contributing to or withdrawing from accounts proportionally to their account balances.

(Consider whether other approaches make sense; would it ever make sense to do so in an ordered way between accounts of the same type? Should TransactionStrategy be extended to support user-defined behaviour when dividing transactions between accounts of the same type? For instance, might we want to allow a user to favour contributions to one plannee's RRSPs over the other's, due to tax considerations? This may need to be split out into a separate issue for a future milestone.)

ChrisCScott commented 6 years ago

Consider revising the weights attribute of TransactionStrategy. Should weights be passed as an input to the strategy method (rather than __init__) and require a dict of {Account: weight} pairs?

This makes TransactionStrategy's strategy methods much easier to write, but puts a burden on client code to build the necessary inputs. It's not clear how Forecast and Forecaster would be revised to accommodate this. It's also beyond the scope of the original Savings Projections model.

At present, I lean away from doing this, but it might be worthwhile to implement an optional weights argument that overrides self.weights. Consider offloading the logic for mapping accounts to weights to a method called in __call__ to avoid overcomplicating each strategy method.

ChrisCScott commented 6 years ago

Initial implementation passes tests without requiring further revision. Issue closed, but see #5 for further changes.