fix: preserve other commodities in partial inclusive balance assignments - #2766
ryanduguid wants to merge 2 commits into
Conversation
|
Thanks @ryanduguid. We still need your first PR to be non-AI-assisted, but this description sounds AI-generated. Am I wrong ? |
Hi Michael, You are not. I'm just an accountant. I will admit that I cannot submit a PR to hledger purely by hand. I do not have the skill set. Your repository is primarily in Haskell, I only have experience with Python, VBA and the Excel formula language as those are easy to learn and understand. If you want to double-check that you are speaking with a human, I can hop on a Discord / Teams / Google / Zoom call with my webcam on or you can email me. My email is hosted on Proton Mail and has no AI connections. You can verify the public records if inclined. If you dislike my PRs, I will ensure my agents never submit one to hledger again. A suitable compromise might be that my agents are only allowed to even attempt to submit a PR to hledger if they meet a certain criterion that you specify, if they are so inclined. For example, it must be reviewed by three different models in different harness environments and must leave the PR descriptions to me. I offer this as a suggestion as I saw an earlier PR was complemented and the capabilities of these models will only improve over time. Apologies for any inconvenience as I understand you take human time to review these. Kind regards from Australia, |
|
Hi Ryan! I will be glad to be able to accept PRs from you. Especially given your accounting knowledge. And your PRs looked to be high quality, but seemingly unaware of our project policies. AI is a sensitive, controversial and polarising topic, and AI-generated submissions can easily derail a volunteer-driven FOSS project (human submissions can too! but AI is much faster). So, we do currently accept AI-assisted PRs, but to keep things sustainable, equitable and at least vaguely principled, we enforce a specific policy. This is our criteria (and it may change from time to time). Perhaps I mentioned it last time, but here it is: and here's a more focussed version aimed at pull requesters: In particular we ask that a contributor's first PR follow these rules. It can be a very small PR. This is a relatively low bar, and it helps to block drive-by auto-submissions which can be very costly. You have a github account and seem quite technical - how about finding some documentation that needs improving and submitting an edit ? You can do this in the github web interface, eg. If needed, you can jump into the hledger chat room and I'll be happy to coach you. |
That's fair enough, I'll see what I can do over the long weekend. Have a good one. |
| getInclusiveRunningBalanceB parent = withRunningBalance $ \BalancingState{bsBalances} -> | ||
| H.foldM | ||
| (\ibal (acc, amt) -> return $ | ||
| if parent==acc || parent `isAccountNamePrefixOf` acc then maPlus ibal amt else ibal) |
There was a problem hiding this comment.
this is some kind of tree math. library?
There was a problem hiding this comment.
It uses the existing Data.HashTable.ST.Cuckoo library and hledger's account-name and amount helpers. H.foldM sums the account's balance and those of its descendants, selected by isAccountNamePrefixOf. This extracts the calculation already used by inclusive assertion checking so assignments can share it. No additional tree library is needed.
| if assertedacct==acc || assertedacct `isAccountNamePrefixOf` acc then maPlus ibal amt else ibal) | ||
| nullmixedamt | ||
| bsBalances | ||
| then getInclusiveRunningBalanceB assertedacct |
There was a problem hiding this comment.
It is the account name on the posting whose balance assertion is being checked. The function header p@Posting{paccount=assertedacct} binds the posting's paccount field to that local name. For an inclusive assertion, the helper sums that account's running balance and its descendants' balances.
3 participants
Summary
Fixes #2093. An amountless
=*posting now changes only the assigned commodity across the account and its subaccounts. For child holdings of 10 EUR and 12 USD, assigning 20 USD generatesassets 8 USDandequity -8 USD, preserving the 10 EUR balance. Previously it also generatedassets -10 EURandequity 10 EUR.The partial assignment now preserves other commodities from the inclusive running balance before passing the complete target to the existing setter. A private helper shares that aggregation with inclusive assertion checking. The manual and changelog explain the scope and historical recalculation.
Evidence
09cd5226passes its 270 existing CLI/library unit tests. Adding the regressions to unchanged production code gives 9 failures among 283 unit tests and 21 failures among 33 new functional cases.After: All 283 CLI/library unit tests and all 33 new functional cases pass. Coverage includes all four operators, repeated assignments, signed and zero balances, nested accounts, account-name boundaries, costs, precision, dates and posting types.
stack96.yaml, the warning-as-error build passes.just testpasses the embedded-file checks, all 2,104 functional cases and all 314 doctest examples.just pkgtestpasses all four packages, including 274 library, 9 CLI and 10 UI unit tests and the web application tests.git diff --checkpasses.74f61a5c, including all four package build/test steps, the functional suite and theweb-e2ebrowser test job.The functional suite ran under the existing unprivileged
nobodyUID with a task-local UTF-8 locale. Running as root without that locale caused two environment failures, both reproduced with the baseline executable.Merge danger
Door: Two-way. Blast radius: Inferred postings.
Re-reading affected journals can change inferred postings, counterpart amounts, later assignments and reports. Review amountless
=*postings on accounts with subaccounts holding other commodities. Use==*when other inclusive commodity balances should be zeroed; it does not generally reproduce the old behaviour when the parent also held those commodities. Explicit adjustments and counterparts preserve exact historical amounts.Each partial inclusive assignment adds one account-table traversal. No dependency or public API changes are required.
Unverified
The default GHC 9.14 toolchain, the rest of the compiler matrix, native Windows/macOS and timing benchmarks have not been run locally. The four changed files were tested through an exact source-byte match with the Linux build. The changed Haskell source scan returned no findings; Haskell-specific SAST coverage is not established.