-
-
Notifications
You must be signed in to change notification settings - Fork 331
imp: add: Verify balance assertions on each posting (#2355) #2356
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
Conversation
Thanks for this! We should try to explore and document how this differs from the behaviour of balance assertions when reading a journal. I think it must be at least a little different, if immediate feedback is desirable (and I agree that it is). Here are some things to check:
|
Sorry for letting this languish for a bit! Some other projects have consumed me. I'm happy to review/document all of this, and will try to do it later this week or early next week. |
(Let's ignore that it's taken me three weeks to resurface....) Doing all of this definitely revealed some bugs! I'll explore fixing them, just wanted to get them documented first:
|
Thanks for the update -
I think we can usually ignore that case - there's no way to write it in journal format. (Explicitly written posting amounts are always single-commodity.) Re posting dates etc, we like to be flexible but we can impose restrictions for sanity, if needed. |
To follow up on some discussions we've had over Matrix, this is mostly done! My last step was investigating adding posting dates. It actually looks like posting dates are not supported by current I could look at adding support for this next, but I think that's outside of the scope of this PR. |
I see. I think the main thing we'd want is that error reporting and balance calculations are the same both inside and outside an Until now posting dates didn't affect an As a simplifying limitation, we could disallow combining a posting date with a balance assertion/assignment when using |
Cool, makes sense to extend it. I still have cleanup to do, but this is working now:
|
7a36d60
to
bf26a09
Compare
Sorry, forgot to run the complete test suite instead of my subset. I'll see why this one fails! |
Sorry about that, embarrassing mistake. This should (finally) be ready now! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good! Comments attached.
@@ -146,6 +148,17 @@ transactionCheckBalanced BalancingOpts{commodity_styles_} t = errs | |||
isTransactionBalanced :: BalancingOpts -> Transaction -> Bool | |||
isTransactionBalanced bopts = null . transactionCheckBalanced bopts | |||
|
|||
-- | Verify that any assertions in this transaction hold | |||
-- when included in the larger journal. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I imagine this could be slow if called for a lot of transactions, but we intend it to be called only for one, the one user has just entered during an add
session. So, minimal user-visible delay even with a big journal and slow machine, hopefully..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmmm. I'm not entirely sure I understand what you're saying, but this does need to be run with the entire journal (since we're checking that the most recent transaction slots into the rest of the journal). For what it's worth, I don't claim to have a huge journal, but hledger stats
says I have over 5000 transactions, and hledger add
with assertions is still quite snappy on my machine.
Perhaps a nice compromise is to only do the assertion checking if there's an actual assertion in the given transaction?
I'll note that one side effect of this PR that I hadn't considered until now is that this will also check that any future assertions still hold as a result of the transaction, and we'd lose that by only entering the assertion logic if there is an assertion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this will also check that any future assertions still hold as a result of the transaction
Aiee! Good point, I hadn't thought of that. We should probably mention in the manual, and I should mention in release notes: entering an amount during add
can now fail if it would break any balance assertion in the journal. (Even if you aren't adding a new assertion.) Also it can fail if there was a pre-existing failing assertion. Though in that case, add
probably wouldn't run - unless you run it with -I
. We should probably make sure that the new assertion check also respects -I
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should probably make sure that the new assertion check also respects
-I
.
I thought that this check would skip any assertions, but a quick test seems to indicate otherwise! I'll check this out as well. I'm happy to update the manual as well, and good call on flagging this in the release notes.
It also appears that I might have been ahead of myself and already implemented the "only check if there's an assertion in the transaction" restriction--let me think a bit more about the ramifications of that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought that this check would skip any assertions [...]
It would help to not just use defbalancingopts
everywhere 🤦♂️
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've corrected it so that -I
works to ignore assertions. With that in place, I think I'll propose removing the restriction of only checking that assertions pass when there's an assertion in the transaction. I added that for performance, but even with the 100ktxns-1kaccts.journal
example file, it takes less than a second to analyze an assertion on my laptop.
This would let us always make sure that the add
ed transaction doesn't break existing assertions, which I think is a nice feature, and with -I
, it can be disabled if it's ever undesirable.
> /Save this transaction to the journal/ | ||
>2 // | ||
|
||
## 13. Assertions with posting dates |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we also test add-ing an amount with an assertion and a posting date ? I'm not sure if that's supported.
Also what about balance assignments, are they expected to work or be rejected ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch! It looks like it's not possible to add an assertion and a posting date such that the assertion would be valid on the day of the posting date. However, this is also not valid in a journal file:
2025-05-01 x
a 50 USD
b -50 USD
2025-05-02 x2
a 100 USD == 50 USD ; date:2025-05-05
c -100 USD
2025-05-10 x3
a 50 USD == 200 USD
d -50 USD
$ hledger print -f test.hledger
hledger: Error: /home/michael/builds/hledger/test.hledger:6:19:
| 2025-05-02 x2
6 | a 100 USD == 50 USD ; date:2025-05-05
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
| c -100 USD
This balance assertion failed.
In account: a
and commodity: USD (no other commodities allowed)
this balance was asserted: 50
but the calculated balance is: 150
This might be possible to support eventually, but since this is already the existing behavior, I think that's better for a follow-up PR.
Balance assignments aren't supported in stock hledger
:
$ hledger add
Date [2025-05-28]:
Description: Opening Balances
Account 1: Assets:Checking
Amount 1: = $409.32
A valid hledger amount is required. Eg: 1, $2, 3 EUR, "4 red apples".
Amount 1:
If we're interested, I could look at adding those next.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
However, this is also not valid in a journal file:
I wouldn't expect that journal file to pass. But this should be accepted, shouldn't it ?
$ hledger -f new.j add
Date [2025-05-28]:
Description:
Account 1: (a)
Amount 1: 10
Account 2 (or . or enter to finish this transaction):
2025-05-28
(a) 10
Save this transaction to the journal ? [y]:
Saved.
Starting the next transaction (. or ctrl-D/ctrl-C to quit)
Date [2025-05-28]:
Description:
Account 1 [(a)]:
Amount 1 [10]: 3 = 3 ; date:5/1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(Equivalent to this journal:)
2025-05-28
(a) 10
2025-05-28
(a) 3 = 3 ; date:5/1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Balance assignments aren't supported in stock hledger:
Right you are, current hledger doesn't allow add-ing balance assertions so of course it doesn't allow add-ing balance assignments either.
Once it allows add-ing assertions, people might expect assignments too, let's mention in the manual that add-ing those isn't supported yet.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But this should be accepted, shouldn't it ?
And yes that works as desired, I just checked. (Except it required me to enter the year; ideally it would use the transaction date's year as default.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it, thanks! I've added a test for this case. Not sure why the partial date doesn't work...I'll dig into what the journal parsing does when handling partial dates.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good! Comments attached.
Hmmm, not really sure why that test failed? All of them pass on my machine ™️... |
The failing test was a real puzzle. Finally it just worked for no apparent reason. I re-ran the PR test job and it too passed. My best guess is that caching went wrong somehow and it wasn't running a freshly built hledger. Now merged. Thanks for seeing this through! |
This prevents `shelltest -w HLEDGERBIN` from disturbing those commands.
Somehow I missed a failure due to incorrect |
This prevents `shelltest -w HLEDGERBIN` from disturbing those commands.
As requested in #2355, this verifies balance assertions during
hledger add
. Here's an example of the new workflow. With the following journal file:The only thing that might be odd here is including the "To troubleshoot..." lines. I left those in because it's not bad advice for tracking down why an assertion might be failing, but also it isn't really helpful and might be misleading in the middle of an
hledger add
, so I'm open to removing those lines here.