dmitry-merzlyakov / nledger

.Net Ledger: Double-Entry Accounting System
Other
182 stars 51 forks source link

Balance calculation fails #5

Open dvabuzyarov opened 4 years ago

dvabuzyarov commented 4 years ago

I have a transaction that is fine in ledger:

2019/07/12 * box 55:assets:box -55 HUMMER @ 5372 EUR 55:expenses:delivery -1,700 EUR 55:expenses:delivery -13,818.44 EUR 55:assets:box 55 HUMMER @ 5,654.15345 EUR

but it fails with nledger The root cause is that the balance of this transaction is visible as -0, and the IsZero property returns false.

        [TestMethod]
        public void Test()
        {
            var amount1 = new Amount(10.16);
            var amount2 = new Amount(-10.15345);
            Value balance = null;
            balance = Value.AddOrSetValue(balance, Value.Get(amount1));
            balance = Value.AddOrSetValue(balance, Value.Get(amount2));
            Assert.IsTrue(balance.IsZero);
        }

I would be happy to assist to sort it out.

dmitry-merzlyakov commented 4 years ago

Hi, thank you for your input, let me take a look, I will respond you in a few days. Basically, I confirm the issue with the test case; it is obviously about rounding. However, I cannot tell for sure which application manages this case less reasonable - I remember some examples when ledger rounding worked not so accurate as it was expected. Let me check, thank you!

dvabuzyarov commented 4 years ago

I would propose to work on the issue in a pair session via Skype. I am going to use this lib in my personal project and expecting to find out some more issues, so it would be very valuable to get some insight directly from you.

dmitry-merzlyakov commented 4 years ago

Hi, it is likely you use another version of ledger that I used for porting: I see that your transaction fails for both nledger and ledger with exactly the same message: "Error: Only one posting with null amount allowed per transaction". My version of ledger version is 3.1.1; here is the output:

dmitry@DESKTOP-A712KAM:~$ ledger Ledger 3.1.1-20160111, the command-line accounting tool

dmitry@DESKTOP-A712KAM:~$ ledger bal -f /mnt/c/dev/dmvs/nledger/source/nledger.cli/bin/debug/issues-5.test While parsing file "/mnt/c/dev/dmvs/nledger/source/nledger.cli/bin/debug/issues-5.test", line 5: Error: Only one posting with null amount allowed per transaction

NLedger produces the same: C:\DEV\DMVS\NLedger\Source\NLedger.CLI\bin\Debug>NLedger-cli.exe bal -f issues-5.test While parsing file "C:\DEV\DMVS\NLedger\Source\NLedger.CLI\bin\Debug\issues-5.test", line 5: Error: Only one posting with null amount allowed per transaction

Currently, NLedger code is equal to ledger 3.1.1 branch NEXT commit fd486a59. Please, check ledger version on your side.

So, I do not consider this issue to be a defect - nledger behaves exactly as its origin. However, if you use a newer version of ledger, I think it is a good time to consider an upgrade for NLedger codebase. Per quick look through ledger commits, I would say that it will take several weeks. Let me know if you are interested in it, it is not a big deal for me.

In regards to the unit test example - I am not sure whether it is expected to work: your numbers are definitely differ, so that they never balance. I tried to imagine how to make it work (e.g. to play with commodities, precisions and flags) but did not fine any practical examples. If your code needs for something similar - I would recommend to create a ledger unit test with similar example - just to validate that ledger code can manage it.

And, of course, any time you want to discuss any aspects of ledger/nledger code - you are welcome; just ping me by skype dmitry.merzlyakov. Thank you for your interest to this stuff!

dmitry-merzlyakov commented 4 years ago

BTW, I created a test file for your case; you can find it in attach issues-5.zip

dvabuzyarov commented 4 years ago

Yes, that is true, I am using a newer version, mine version is Ledger 3.1.3-20190331. The idea to update the codebase sounds good for me.

dmitry-merzlyakov commented 4 years ago

Hi, I have an update in regards to this ticket:

  1. I updated NLedger code base with the latest changes in ledger; now NLedger "next-dev" branch exactly matches ledger 3.1.3 (ledger master commit 7c45da22 - 3/19/2020 5:57:49 AM). You can check _CI.BuildLog.md to get the latest binaries;
  2. I created a ledger-style test for your transaction - \NLedger\Contrib\test\nledger\gh-issues-5.test

The point is that this test shows the same behavior for both NLedger and the latest ledger - an attempt to read your transaction leads to the error: Error: Only one posting with null amount allowed per transaction

(The original ledger was built with the latest master sources on a fresh image of Ubuntu Server 19.10 following recommended steps).

So, I do not see any discrepancies between ledger and NLedger so far; your transaction causes an expected error.

If you do no think so, let's try the following steps:

  1. Please, make sure that you provided a complete transaction text (check gh-issues-5.test). If it is not complete - please, send me an updated test;
  2. If transaction text is OK - please, check that gh-issues-5.test does not cause an error for your ledger. In this case, we need to troubleshoot deeper why your ledger behaves differently than mine.

Thank you!

dvabuzyarov commented 4 years ago

It is a really good work. I will try it on the next weekend.

Thanks!

alensiljak commented 1 year ago

It's been a few years. Is this issue still valid?

The error about empty values seems strange as all the postings have values. However, the IsZero issue may be the same as the one just fixed in #29? Two birds, one stone story.

dmitry-merzlyakov commented 1 year ago

I think yes, this issue is still actual. There is a related test file - \Contrib\test\nledger\gh-issues-5.test; it reflects that the described transaction causes an error in NLedger:

2019/07/12 * box
     55:assets:box -55 HUMMER @ 5372 EUR
     55:expenses:delivery -1,700 EUR
     55:expenses:delivery -13,818.44 EUR
     55:assets:box 55 HUMMER @ 5,654.15345 EUR

test bal -> 1
__ERROR__
While parsing file "$FILE", line 5:
Error: Only one posting with null amount allowed per transaction
end test

I do not have the latest Ledger version handy, but I believe that 3.2.1-20200518 produced exactly the same error. It would be good to check what the latest version produces. If it is not an issue for it anymore - their fixing changes will be included into NLedger when I apply recent c++ code to NLedger.

Anyway, it is not related to 29 because the latter is a mistake in .Net code.

alensiljak commented 1 year ago

Initially I thought the issue was due to wrong display on GitHub, using HTML, so I downloaded the test file. One thing that seems immediately obvious is that all the postings have the amounts. And the amounts are separated with only one space character. I don't know if this has something to do with me looking at it on the phone. But if it is really one space character, then the amounts are considered the part of the account name.

I've just added the second space to separate the amounts from the account name and the result is the following

~/.../shared/Documents $ ledger -f gh-issues-5.txt b
      -15,518.44 EUR  55:expenses:delivery
alensiljak commented 1 year ago

It looks like the syntax in that file is wrong. It is not a bug in Ledger or NLedger.

alensiljak commented 1 year ago

The correct syntax for the Xact would be:

2019/07/12 * box
     55:assets:box  -55 HUMMER @ 5372 EUR
     55:expenses:delivery  -1,700 EUR
     55:expenses:delivery  -13,818.44 EUR
     55:assets:box  55 HUMMER @ 5,654.15345 EUR

The test case gh-issues-5.test should be updated.