New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
1.29: Valid entries with equity conversion postings are rejected if hledger can't detect balancing postings #2045
Comments
Strangely, I don't recall having had an issue with |
The problem occurs during this new pass that is done to allow redundant costs + conversion postings: hledger/hledger-lib/Hledger/Read/Common.hs Line 334 in c2c668e
in addCostsToPostings, when dryrun' is true:
If I make that case do nothing instead, it fixes @the-solipsist's example but breaks the three tests starting at costs test 39: hledger/hledger/test/journal/costs.test Line 510 in c2c668e
I for one need a fresh mind to see when exactly it should raise an error here. |
Since 1.29, we unconditionally run part of the --infer-cost logic to identify redundant costs/equity postings. This was too strict, raising an error whenever it could not find postings matching the equity postings. Now we do this (and also the explicit --infer-costs operation) as a best effort, leaving transactions unchanged if we can't detect matching postings. This is consistent with --infer-equity, --infer-market-prices, -B and -V.
I have pushed a fix (3357c27, meant to push a PR instead..). This changes the transaction-balancing and --infer-cost behaviour of 1.29 and 1.30, to be more forgiving, doing nothing rather than raise errors, which solves the present issue and I think will not cause problems, and is consistent with --infer-equity, infer-market-prices, -B and -V. |
Since 1.29, we unconditionally run part of the --infer-cost logic to identify redundant costs/equity postings. This was too strict, raising an error whenever it could not find postings matching the equity postings. Now we do this (and also the explicit --infer-costs operation) as a best effort, leaving transactions unchanged if we can't detect matching postings. This is consistent with --infer-equity, --infer-market-prices, -B and -V.
Comments welcome. If no-one sees a problem with this we should probably release it in hledger-1.30.2. @the-solipsist you might be able to test the binary at https://github.com/simonmichael/hledger/actions/runs/5196929865. |
Is there any other way of figuring out which journal entries aren't in a format that This is going from a bug report to a feature wish, but I think the 'feature' aspect of the bug needs to be retained in a documented fashion. |
@the-solipsist I haven't experienced it yet but I'm assuming two ways: either the transaction will fail to balance,
We could certainly add the strict error checking back as an on-demand check. Possibly included in the --strict checks.
Noted. That's a difficult feature, perhaps there's an open issue for it.
Which feature aspect is that ? @Xitian9 , any thoughts on this (relaxed error checking when matching equity conversion postings with regular postings) ? To review: Currently, the valid entry @the-solipsist showed here is too similar to an entry that's invalid for inferring cost-from-equity; hledger can't distinguish them. Also, hledger reuses part of the cost-from-equity logic as part of basic journal checking (to help it accept redundant costs). So it wrongly raises an error with this example, even when --infer-cost is not used. The easiest fix is to (1) relax the error checking, making cost-from-equity inference best-effort If 1 is too problematic, some alternatives are: (2) Declare that entries like this example are now invalid, and require humans to write conversion entries in a form that (3) Bring back the stricter error checking, as a check that can be run on demand with (4) Make hledger's inferring cost-from-equity smarter, so it can handle this example. (5) Separate the logic for checking-for-redundant-costs and inferring-cost-from-equity. (6) ? |
Also, for the larger picture, we should have a global sense of
and ideally keep these fairly consistent across the product. |
|
1 (ignoring transactions with unmatchable conversion equity postings) is the current fix. |
By which I mean: "reminder, this is the fix currently in master which will be in the next bugfix release one of these days; more discussion, examples of problems with it, and PRs still welcome." |
@the-solipsist I forgot to mention: please claim your https://hledger.org/regressionbounty . |
Thanks, @simonmichael. I submitted a claim. |
This entry is parsed without issue with
hledger bal
in v1.28:However, this is no longer the case with v1.29+, which throws up the error:
This is essentially doing a strict
--infer-costs
check, rather than / in addition to whatever checks the journal parsing code was doing earlier.This works fine:
The text was updated successfully, but these errors were encountered: