Support multiple portfolios and cash positions#2
Conversation
Cash holdings can now be tracked alongside stock positions. Each cash
position is stored by currency and included in the USD total using live
forex rates.
New CLI commands:
$ stonks add-cash EUR 5000
$ stonks remove-cash EUR 2000
YAML format:
portfolio:
positions:
- symbol: AAPL
quantity: 10
avg_cost: 150.00
currency: USD
cash:
- currency: USD
amount: 5000.00
- currency: EUR
amount: 3000.00
Signed-off-by: Igor Opaniuk <igor.opaniuk@gmail.com>
The total value row was hardcoded to USD. Now each portfolio file can
declare its own base currency so the dashboard reflects the user's
preferred reporting currency.
Example:
portfolio:
base_currency: EUR
positions:
- symbol: ASML.AS
quantity: 5
avg_cost: 680.00
currency: EUR
cash:
- currency: EUR
amount: 3000.00
Defaults to USD when the field is absent (backwards-compatible).
Signed-off-by: Igor Opaniuk <igor.opaniuk@gmail.com>
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces significant new functionality by adding support for multiple portfolios and cash positions. The changes are extensive, touching the data models, storage layer, CLI commands, and the TUI application. The implementation is generally of high quality, with good separation of concerns and comprehensive test coverage for the new features.
I've identified a few areas for improvement in the CLI handling of multiple portfolios to enhance usability and prevent potential user errors. Specifically, I've commented on some dead code, a redundant check, and ambiguous behavior in commands that should only operate on a single portfolio. Overall, this is a great enhancement to the application.
| @click.pass_context | ||
| def add_cash(ctx: click.Context, currency: str, amount: float) -> None: | ||
| """Add AMOUNT of CURRENCY cash to the portfolio.""" | ||
| store: PortfolioStore = ctx.obj["store"] |
There was a problem hiding this comment.
The add-cash command (and also remove-cash, add, remove) implicitly operates on the first portfolio specified with the -p option when multiple are provided. This can be confusing and lead to unintended modifications. To prevent this, you should add a check to ensure only one portfolio is specified for these commands. I'd also recommend updating the docstring to clarify this behavior.
| store: PortfolioStore = ctx.obj["store"] | |
| stores: list[PortfolioStore] = ctx.obj["stores"] | |
| if len(stores) > 1: | |
| raise click.UsageError( | |
| "The 'add-cash' command supports only one portfolio at a time." | |
| ) | |
| store: PortfolioStore = ctx.obj["store"] |
Each portfolio can be now displayed in its own labelled section so positions remain clearly separated. Portfolios also gain a name field in YAML so the section header shows a meaningful label instead of a generic "Portfolio N" fallback: portfolio: name: Work base_currency: USD positions: ... The app title is updated from "Portfolio" to "Stonks". Multiple portfolios can be passed in several ways. Full paths: $ stonks -p ~/finance/work.yaml -p ~/finance/personal.yaml dashboard Shorthand names (resolves to ~/.config/stonks/<name>.yaml): $ stonks -p work -p personal dashboard Mixed: $ stonks -p work -p /mnt/shared/spouse.yaml dashboard Omitting -p entirely falls back to ~/.config/stonks/portfolio.yaml. Signed-off-by: Igor Opaniuk <igor.opaniuk@gmail.com>
igoropaniuk
force-pushed
the
feat/multiple_portfolio_cash
branch
from
5cbb7bc to
cb803d1
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request adds significant new functionality by introducing support for multiple portfolios and cash positions. The changes are well-structured across the model, storage, CLI, and application layers. The introduction of new CLI commands for cash management and portfolio listing is a great addition. The test coverage for the new features is comprehensive. I've identified a minor performance improvement opportunity and a potential point of confusion in the CLI's behavior when handling multiple portfolios with modification commands. Overall, this is a solid contribution.
| else: | ||
| stores = [PortfolioStore(path=_resolve_portfolio_path(p)) for p in portfolio] | ||
| ctx.obj["stores"] = stores | ||
| ctx.obj["store"] = stores[0] |
There was a problem hiding this comment.
Storing only the first portfolio store in ctx.obj["store"] makes commands like add, remove, add-cash, and remove-cash implicitly operate on the first portfolio provided with -p. This can be confusing and lead to unintended modifications when a user specifies multiple portfolios. Consider making these commands either explicitly target a portfolio (e.g., with an additional option) or fail when multiple portfolios are provided, to avoid ambiguity.
| all_symbols = list( | ||
| {p.symbol for portfolio in self.portfolios for p in portfolio.positions} | ||
| ) | ||
| extended = fetcher.fetch_extended_prices(all_symbols) | ||
| new_prices = {sym: price for sym, (price, _) in extended.items()} | ||
| new_sessions = {sym: sess for sym, (_, sess) in extended.items()} | ||
| currencies = list({p.currency for p in self.portfolio.positions}) | ||
| new_forex = fetcher.fetch_forex_rates(currencies) | ||
| all_currencies = list( | ||
| {p.currency for portfolio in self.portfolios for p in portfolio.positions} | ||
| | {c.currency for portfolio in self.portfolios for c in portfolio.cash} | ||
| ) |
There was a problem hiding this comment.
To improve efficiency, you can avoid iterating through all portfolio positions twice (once for symbols, once for currencies). You can collect all positions in a single pass, then extract symbols and currencies from that list.
| all_symbols = list( | |
| {p.symbol for portfolio in self.portfolios for p in portfolio.positions} | |
| ) | |
| extended = fetcher.fetch_extended_prices(all_symbols) | |
| new_prices = {sym: price for sym, (price, _) in extended.items()} | |
| new_sessions = {sym: sess for sym, (_, sess) in extended.items()} | |
| currencies = list({p.currency for p in self.portfolio.positions}) | |
| new_forex = fetcher.fetch_forex_rates(currencies) | |
| all_currencies = list( | |
| {p.currency for portfolio in self.portfolios for p in portfolio.positions} | |
| | {c.currency for portfolio in self.portfolios for c in portfolio.cash} | |
| ) | |
| all_positions = [ | |
| p for portfolio in self.portfolios for p in portfolio.positions | |
| ] | |
| all_symbols = list({p.symbol for p in all_positions}) | |
| extended = fetcher.fetch_extended_prices(all_symbols) | |
| new_prices = {sym: price for sym, (price, _) in extended.items()} | |
| new_sessions = {sym: sess for sym, (_, sess) in extended.items()} | |
| all_currencies = list( | |
| {p.currency for p in all_positions} | |
| | {c.currency for portfolio in self.portfolios for c in portfolio.cash} | |
| ) |
1 participant
No description provided.