Skip to content

Conversation

@adept
Copy link
Collaborator

@adept adept commented Dec 19, 2025

This checks for:

At the moment both of these cases fail with undecipherable error from the bowels of IRR computation. Let's fail with a sensible error message instead, and provide a path to resolution, if possible

@adept adept force-pushed the roi-better-input-checks branch from 1331c59 to 3edd9cd Compare December 20, 2025 00:05
@adept adept force-pushed the roi-better-input-checks branch from 3edd9cd to c8c62dc Compare December 20, 2025 01:14
@simonmichael
Copy link
Owner

simonmichael commented Dec 25, 2025

This looks like a great error message improvement. Does it really fix #2505 as the message states ? That was the following not-so-clear message:

> stack run hledger -- roi --file="/tmp/roi.journal" --value="then,€" --inv "^assets:.*:TICK$" --pnl ""
hledger: Error: Error (NotBracketed): No solution for Internal Rate of Return (IRR).

@adept
Copy link
Collaborator Author

adept commented Dec 25, 2025

Yep, with --pnl "" (which is parsed as Any) you get:

hledger: Error: Need some transactions classed as investment and not pnl, but the pnl query matches any transaction - will be unable to compute the rates of return

@adept
Copy link
Collaborator Author

adept commented Dec 25, 2025

And if you fix that and instead, say, omit` --value, you get:

> stack run hledger -- roi --file="/tmp/roi.journal" --inv "^assets:.*:TICK$" --pnl "__none__"
hledger: Error: Period (2025-08-14,2025-10-24) has multiple commodities: TICK, €
Consider using --value to force all costs to be in a single commodity.
For example, "--value=end,<commodity> --infer-market-prices", where commodity is the one that you want to value the investment in.

@simonmichael
Copy link
Owner

simonmichael commented Dec 26, 2025

The #2505 error message is still present in the code though.

It has more helpful additional text than I noticed before. Perhaps we can just remove the "Error (NotBracketed): " prefix there, as Error is is already printed by hledger and NotBracketed isn't needed by users (likewise for the line below).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants