Skip to content
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

transaction modifiers miss inferred amounts in transaction #893

Open
jkr opened this issue Oct 9, 2018 · 18 comments
Open

transaction modifiers miss inferred amounts in transaction #893

jkr opened this issue Oct 9, 2018 · 18 comments

Comments

@jkr
Copy link
Contributor

@jkr jkr commented Oct 9, 2018

Running with a stack build of up-to-date master.

Transaction modifiers seem to interact oddly with inferred amounts. The following works as expected:

= ^expenses:groceries
    [budget:groceries]                           *-1
    [assets:bank:checking]                        *1

10/7 * MARKET
    expenses:groceries:food                      $20
    assets:bank:checking
$ hledger -ffoo.ledger reg --auto
2018/10/07 MARKET               ex:groceries:food              $20           $20
                                [budget:groceries]            $-20             0
                                [as:bank:checking]             $20           $20
                                assets:bank:checking          $-20             0

But the following does not:

= ^expenses:groceries
    [budget:groceries]                           *-1
    [assets:bank:checking]                        *1

10/7 * MARKET
    expenses:groceries:food
    assets:bank:checking                         $-20
$ hledger -ffoo.ledger reg --auto
hledger: could not balance this transaction - can't have more than one balanced virtual posting with no amount (remember to put 2 or more spaces before amounts)
2018/10/07 * MARKET
    expenses:groceries:food
    [budget:groceries]
    [assets:bank:checking]
    assets:bank:checking                $-20
@simonmichael simonmichael changed the title transaction modifiers miss some accounts in transaction transaction modifiers miss inferred amounts in transaction Oct 9, 2018
@simonmichael
Copy link
Owner

@simonmichael simonmichael commented Oct 9, 2018

Thanks for the report.

@jkr
Copy link
Contributor Author

@jkr jkr commented Oct 9, 2018

I'll poke around as a way of familiarizing myself with the codebase, and see if I can help out. Would HLedger.Data.TransactionModifer be where I should start? (It looks like the inference logic is in H.D.Transaction, right?)

@simonmichael
Copy link
Owner

@simonmichael simonmichael commented Oct 9, 2018

@jkr, that would be great. Yes that sounds right.

@jkr
Copy link
Contributor Author

@jkr jkr commented Oct 10, 2018

I figured out the issue: it was applying the modifiers before it finalized the journal with journalFinalise. A simple solution (apply modifiers after journalFinalise) is here:

[EDIT: this solution doesn't work, for the reasons discussed in this post. There is a better solution in the post below.]

master...jkr:modifiers_fix

Now this works:

= ^expenses:groceries
    [budget:groceries]                           *-1
    [assets:bank:checking]                        *1

10/7 * MARKET
    expenses:groceries:food                      $20
    assets:bank:checking

10/8 * MARKET2
    expenses:groceries:food
    assets:bank:checking                        -$20
$ stack exec hledger -- print -f /tmp/foo.ledger --auto
2018/10/07 * MARKET
    expenses:groceries:food             $20
    [budget:groceries]                 $-20
    [assets:bank:checking]              $40
    assets:bank:checking               $-20

$ stack exec hledger -- print -f /tmp/foo.ledger --auto
2018/10/07 * MARKET
    expenses:groceries:food             $20
    [budget:groceries]                 $-20
    [assets:bank:checking]              $20
    assets:bank:checking

2018/10/08 * MARKET2
    expenses:groceries:food
    [budget:groceries]                 $-20
    [assets:bank:checking]              $20
    assets:bank:checking               $-20

BUT, this produces its own problem. Because the application happens after the finalization, it no longer checks to make sure they balance. So we can now have:

= ^expenses:groceries
    [budget:groceries]                           *-1
    [assets:bank:checking]                        *2

10/7 * MARKET
    expenses:groceries:food                      $20
    assets:bank:checking                        -$20
$ stack exec hledger -- print -f /tmp/foo.ledger --auto
2018/10/07 * MARKET
    expenses:groceries:food             $20
    [budget:groceries]                 $-20
    [assets:bank:checking]              $40
    assets:bank:checking               $-20

So it seems like the best option would be to run finalize twice: before and after modification. This makes the most intuitive sense since the journal should work both with and without --auto, so it's essentially checking two different journals. I'm not sure how expensive this would be, but the second run would only occur if auto_ iopts, so if you don't run specifiy auto you won't see it.

I could also try to split out infernce from finalize, but I imagine there are other checks that would end up missing, so running it twice if you have auto seems like the most direct route.

Thoughts?

Also: when do you use doctests and when do you use easytests? I'll add tests when i make a PR but I'm not sure what you usually do.

@jkr
Copy link
Contributor Author

@jkr jkr commented Oct 10, 2018

Okay, this implements the double-run I discussed above:

master...jkr:modifiers_fix_2

What do you think?

@simonmichael
Copy link
Owner

@simonmichael simonmichael commented Oct 10, 2018

That sounds reasonable.

@jkr
Copy link
Contributor Author

@jkr jkr commented Oct 10, 2018

OK -- I put up a PR. There's no testing in at the moment, since I couldn't figure out the proper place to put this test in the current framework. If you have a suggestion, I'll add it to the PR.

@jkr
Copy link
Contributor Author

@jkr jkr commented Oct 10, 2018

I see from the Travis output where the relevant tests are. I'll try to address those and add to the PR.

jkr added a commit to jkr/hledger that referenced this issue Oct 11, 2018
Currently, automated transactions are added before the journal is
finalized. This means that no inferred values will be picked up. We
change the procedure, if `auto_` is set, to

 1. first run `journalFinalise` without assertion checking (assertions
    might be wrong until automated transactions), but with reordering
 2. Insert transaction modifiers
 3. Run `journalFinalise` again, this time with assertion checking as
    set in the options, and without reordering.

If `auto_` is not set, all works as before.

Closes: simonmichael#893
@simonmichael
Copy link
Owner

@simonmichael simonmichael commented Oct 12, 2018

Seems good to me! Thanks for the fix.

@thargor
Copy link

@thargor thargor commented Jan 30, 2019

Hi @jkr ,
after talking to @simonmichael on irc we decided to document this here and maybe reopen this issue:

My use case is double checking tax calculations by supplying the net amount, gross amount and taxation rules as tags.

The following example works without these pull requests (<1.12) and breaks after (>= 1.12).

example.txt

= tag:tax20
  taxes  *0.2


2018/12/18 transaction with taxes
  a    EUR -10.00    ; :tax20:
  b    EUR 12.00

before 1.12

$ ./hledger.exe -f  example.txt bal --auto
          EUR -10.00  a
           EUR 12.00  b
           EUR -2.00  taxes
--------------------
                   0

after 1.12

hledger.exe: "example.txt" (lines 5-7)
could not balance this transaction (real postings are off by EUR 2.00)
2018/12/18 transaction with taxes
    a      EUR -10.00    ; :tax20:
    b       EUR 12.00

Before 1.12 it was not required to have the transactions balance before adding the automated transactions and this worked.

@thargor
Copy link

@thargor thargor commented Jan 30, 2019

This does also not work with 1.12:

= tag:tax20  
  taxes  *0.2
  
  
2018/12/18 transaction with taxes
  a    EUR -10.00    ; :tax20:
  b
@simonmichael
Copy link
Owner

@simonmichael simonmichael commented Jan 31, 2019

Thanks for these examples @thargor. Here they are in another form:

A. Both amounts explicit:

= tag:tax20
  taxes  *0.2

2018/12/18 transaction with taxes
  a    EUR -10.00    ; :tax20:
  b    EUR 12.00


# 1. hledger 1.11 accepts this, with --auto
$ hledger-1.11 print -x --auto
2018/12/18 transaction with taxes
    a          EUR -10.00    ; :tax20:
    taxes       EUR -2.00
    b           EUR 12.00

# 2. hledger 1.12 doesn't
$ hledger-1.12 print -x --auto
hledger-1.12: "/Users/simon/src/PLAINTEXTACCOUNTING/hledger/b.j" (lines 5-7)
could not balance this transaction (real postings are off by EUR 2.00)
2018/12/18 transaction with taxes
    a      EUR -10.00    ; :tax20:
    b       EUR 12.00

B. One amount implicit:

= tag:tax20  
  taxes  *0.2
  
2018/12/18 transaction with taxes
  a    EUR -10.00    ; :tax20:
  b


# 3. hledger 1.11 accepts this with or without --auto
$ hledger-1.11 print -x
2018/12/18 transaction with taxes
    a      EUR -10.00    ; :tax20:
    b       EUR 10.00

# 4.
$ hledger-1.11 print -x --auto
2018/12/18 transaction with taxes
    a          EUR -10.00    ; :tax20:
    taxes       EUR -2.00
    b           EUR 12.00

# 5. hledger 1.12 accepts it only without --auto
$ hledger-1.12 print -x
2018/12/18 transaction with taxes
    a      EUR -10.00    ; :tax20:
    b       EUR 10.00

# 6.
$ hledger-1.12 print -x --auto
hledger-1.12: "/Users/simon/src/PLAINTEXTACCOUNTING/hledger/c.j" (lines 5-7)
could not balance this transaction (real postings are off by EUR -2.00)
2018/12/18 transaction with taxes
    a          EUR -10.00    ; :tax20:
    taxes       EUR -2.00
    b           EUR 10.00

@thargor always runs with --auto, and wanted to write both amounts explicitly for clarity/error checking (example 1).

@jkr's original examples above seemed sensible, but now the failing examples 2 and 6 with hledger 1.12 do seem a bit wrong. Perhaps we need to rethink this change. Even revert it ? To do something in time for tomorrow's release, it will have to happen soon. @jkr or anyone, any thoughts ?

@simonmichael simonmichael reopened this Jan 31, 2019
@simonmichael
Copy link
Owner

@simonmichael simonmichael commented Jan 31, 2019

the journal should work both with and without --auto

Is this possible/desirable as a general rule ? It seems not possible for some journals, eg A above (unless you did a looser check, trying to balance transactions both with and without --auto and accepting either.)

@simonmichael
Copy link
Owner

@simonmichael simonmichael commented Jan 31, 2019

1.11 required transactions to balance after any auto postings had been added.
1.12+ requires transactions to also balance before auto postings are added.

@simonmichael
Copy link
Owner

@simonmichael simonmichael commented Jan 31, 2019

To comply with that, here's how @thargor would have to write that journal A:

C. balanced both before and after auto postings added

= tag:tax20
  taxes   *0.2
  b      *-0.2

2018/12/18 transaction with taxes
  a    EUR -10.00    ; :tax20:
  b     EUR 10.00

# 7.
$ hledger-1.12 print -x
2018/12/18 transaction with taxes
    a      EUR -10.00    ; :tax20:
    b       EUR 10.00

# 8.
$ hledger-1.12 print -x --auto
2018/12/18 transaction with taxes
    a          EUR -10.00    ; :tax20:
    taxes       EUR -2.00
    b            EUR 2.00
    b           EUR 10.00
@thargor
Copy link

@thargor thargor commented Jan 31, 2019

= tag:tax20
  taxes   *0.2
  b      *-0.2

I cannot use this, since b (and also a) is not a fixed accout name, but depends on the transaction.

2018/12/18 transaction with taxes 1
  a    EUR -10.00    ; :tax20:
  b     EUR 10.00

2018/12/18 transaction with taxes 2
  c    EUR -10.00    ; :tax20:
  d     EUR 10.00
@simonmichael
Copy link
Owner

@simonmichael simonmichael commented Jan 31, 2019

I cannot use this, since b (and also a) is not a fixed accout name, but depends on the transaction.

Could be handled with multiple more specific rules, I assume:

= tag:tax20  desc:'taxes 1'
  taxes   *0.2
  b      *-0.2

= tag:tax20  c  ; or whatever
  taxes   *0.3
  d      *-0.3

Unrelated: I see you're using Ledger tag syntax - hledger doesn't require that leading colon.

simonmichael added a commit that referenced this issue Feb 1, 2019
simonmichael added a commit that referenced this issue Feb 1, 2019
simonmichael added a commit that referenced this issue Feb 1, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
3 participants