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

Wrong multi-currency printing #465

Closed
rmoehn opened this Issue Jan 10, 2017 · 11 comments

Comments

Projects
None yet
3 participants
@rmoehn

rmoehn commented Jan 10, 2017

I'm not sure if I'm doing the right thing, but I think I've found a problem. I'm trying to use a currency trading account like this:

2017-01-07 buy food
    expenses:food   20 usd @ 1.30 cad
    assets:usd     -20 usd
    income:currency

hledger print yields:

2017/01/07 buy food
    expenses:food    20 usd @ 1.30 cad
    assets:usd                 -20 usd
                            -26.00 cad
    income:currency             20 usd

Using hledger print -O csv, I can understand how this is meant:

"txnidx","date","date2","status","code","description","comment","account","amount","commodity","credit","debit","status","posting-comment"
"1","2017/01/07","","","","buy food","","expenses:food","20 @ 1.30 cad","usd","","20 @ 1.30 cad","",""
"1","2017/01/07","","","","buy food","","assets:usd","-20","usd","20","","",""
"1","2017/01/07","","","","buy food","","income:currency","20","usd","","20","",""
"1","2017/01/07","","","","buy food","","income:currency","-26.00","cad","26.00","","",""

However, in this case print is not idempotent. – hledger print | hledger -f- print yields:

2017/01/07 buy food
    expenses:food    20 usd @ 1.30 cad
    assets:usd                 -20 usd
    -26.00 cad              -26.00 cad
    income:currency             20 usd

This looks wrong.

@simonmichael

This comment has been minimized.

Show comment
Hide comment
@simonmichael

simonmichael Jan 10, 2017

Owner

Thanks for the report. Yes, print's output is not always valid journal format, which is a pity. Postings where the amount is implicit can have a multi-commodity amount. To show this in valid journal format would mean showing one posting per commodity, like this:

2017-01-07 buy food
    expenses:food     20 usd @ 1.30 cad
    assets:usd       -20 usd
    income:currency   20 usd
    income:currency  -20 usd @ 1.30 cad

Should the print command do this ? Ie, show a journal entry which is not what you wrote, to get valid journal output ?

Or, it could just leave the amount implicit as it was in the first place ?

2017-01-07 buy food
    expenses:food     20 usd @ 1.30 cad
    assets:usd       -20 usd
    income:currency

It originally did this, but I had some reason to change it - clarity when troubleshooting ? See also #442 .

Owner

simonmichael commented Jan 10, 2017

Thanks for the report. Yes, print's output is not always valid journal format, which is a pity. Postings where the amount is implicit can have a multi-commodity amount. To show this in valid journal format would mean showing one posting per commodity, like this:

2017-01-07 buy food
    expenses:food     20 usd @ 1.30 cad
    assets:usd       -20 usd
    income:currency   20 usd
    income:currency  -20 usd @ 1.30 cad

Should the print command do this ? Ie, show a journal entry which is not what you wrote, to get valid journal output ?

Or, it could just leave the amount implicit as it was in the first place ?

2017-01-07 buy food
    expenses:food     20 usd @ 1.30 cad
    assets:usd       -20 usd
    income:currency

It originally did this, but I had some reason to change it - clarity when troubleshooting ? See also #442 .

@simonmichael

This comment has been minimized.

Show comment
Hide comment
@simonmichael

simonmichael Jan 10, 2017

Owner

Let's revert to the old behaviour and see what happens. I think this means print will show all entries with no explicit amount on the last posting - whether you wrote one in the journal entry or not. So eg if you did write all amounts explicitly:

2017-01-07 buy food
    expenses:food     20 usd @ 1.30 cad
    assets:usd       -20 usd
    income:currency   20 usd
    income:currency  -20 usd @ 1.30 cad

print will show

2017/01/07 buy food
    expenses:food     20 usd @ 1.30 cad
    assets:usd                  -20 usd
    income:currency              20 usd
    income:currency

Maybe this is ok. Maybe we'll need a --explicit option later.

Owner

simonmichael commented Jan 10, 2017

Let's revert to the old behaviour and see what happens. I think this means print will show all entries with no explicit amount on the last posting - whether you wrote one in the journal entry or not. So eg if you did write all amounts explicitly:

2017-01-07 buy food
    expenses:food     20 usd @ 1.30 cad
    assets:usd       -20 usd
    income:currency   20 usd
    income:currency  -20 usd @ 1.30 cad

print will show

2017/01/07 buy food
    expenses:food     20 usd @ 1.30 cad
    assets:usd                  -20 usd
    income:currency              20 usd
    income:currency

Maybe this is ok. Maybe we'll need a --explicit option later.

simonmichael added a commit that referenced this issue Jan 10, 2017

print: omit the last posting amount, as we used to #465, #442
This avoids printing invalid journal format for entries where an implicit amount has multiple commodities.
@simonmichael

This comment has been minimized.

Show comment
Hide comment
@simonmichael

simonmichael Jan 10, 2017

Owner

And I shouldn't have pushed that before updating tests. Oh well.

Owner

simonmichael commented Jan 10, 2017

And I shouldn't have pushed that before updating tests. Oh well.

@simonmichael

This comment has been minimized.

Show comment
Hide comment
@simonmichael

simonmichael Jan 10, 2017

Owner

Also, as @ony pointed out this doesn't fix the problem. Eg we still have:

2017-01-07 buy food
    expenses:food   20 usd @ 1.30 cad
    assets:usd     -20 usd
    income:currency
    expenses:fee     3 usd
$ hledger -f 465.journal print
2017/01/07 buy food
    expenses:food    20 usd @ 1.30 cad
    assets:usd                 -20 usd
                            -26.00 cad
    income:currency             17 usd
    expenses:fee

Discussing a bit on IRC.

Owner

simonmichael commented Jan 10, 2017

Also, as @ony pointed out this doesn't fix the problem. Eg we still have:

2017-01-07 buy food
    expenses:food   20 usd @ 1.30 cad
    assets:usd     -20 usd
    income:currency
    expenses:fee     3 usd
$ hledger -f 465.journal print
2017/01/07 buy food
    expenses:food    20 usd @ 1.30 cad
    assets:usd                 -20 usd
                            -26.00 cad
    income:currency             17 usd
    expenses:fee

Discussing a bit on IRC.

ony added a commit to ony/hledger that referenced this issue Jan 10, 2017

WIP: Ensure showTransaction output a valid journal
- Make output of print to be a valid journal
- TODO: Partially reverts 419f5f2

Fixes simonmichael#465

ony added a commit to ony/hledger that referenced this issue Jan 10, 2017

Ensure showTransaction produce a valid journal
- Make output of print to be a valid journal
- Partially reverts 419f5f2

Fixes simonmichael#465
@rmoehn

This comment has been minimized.

Show comment
Hide comment
@rmoehn

rmoehn Jan 11, 2017

Thanks for the quick action!

For my purposes it would be enough if there was a canonical format that I can thread through multiple calls. Like hledger-rewrite -f … | hledger-rewrite -f- … | hledger-rewrite -f- … | hledger -f- print. I thought the CSV format would work, but apparently I misunderstood the purpose of CSV reading and writing. It's thought for communicating with outside programs, not for communicating with hledger itself, right?

rmoehn commented Jan 11, 2017

Thanks for the quick action!

For my purposes it would be enough if there was a canonical format that I can thread through multiple calls. Like hledger-rewrite -f … | hledger-rewrite -f- … | hledger-rewrite -f- … | hledger -f- print. I thought the CSV format would work, but apparently I misunderstood the purpose of CSV reading and writing. It's thought for communicating with outside programs, not for communicating with hledger itself, right?

@simonmichael

This comment has been minimized.

Show comment
Hide comment
@simonmichael

simonmichael Jan 11, 2017

Owner

Yeah. The CSV output of print has one record per posting, while we expect CSV input to have one record per transaction, so doing that with CSV sounds hard.

Owner

simonmichael commented Jan 11, 2017

Yeah. The CSV output of print has one record per posting, while we expect CSV input to have one record per transaction, so doing that with CSV sounds hard.

simonmichael added a commit that referenced this issue Jan 11, 2017

Ensure showTransaction produce a valid journal (#466)
- Make output of print to be a valid journal
- Partially reverts 419f5f2

Fixes #465
@simonmichael

This comment has been minimized.

Show comment
Hide comment
@simonmichael

simonmichael Jan 11, 2017

Owner

Thanks for the PR! I believe this issue is now resolved in master.

Owner

simonmichael commented Jan 11, 2017

Thanks for the PR! I believe this issue is now resolved in master.

@rmoehn

This comment has been minimized.

Show comment
Hide comment
@rmoehn

rmoehn Jan 11, 2017

Thanks! I will try it out soon.

rmoehn commented Jan 11, 2017

Thanks! I will try it out soon.

@ony

This comment has been minimized.

Show comment
Hide comment
@ony

ony Jan 11, 2017

Collaborator

Though journal now is valid I've got strange output for journal

2016/1/1
    saving-card  $-105
    snacks  95 EUR @@ $100
    Equity:Unbalanced  ; absence of $5 here triggers strange behavior
2016/01/01
    saving-card                 $-105
    snacks             95 EUR @@ $100
    Equity:Unbalanced            $105    ; absence of $5 here triggers strange behavior
    Equity:Unbalanced           $-100    ; absence of $5 here triggers strange behavior

I would understand if the output would look like:

2016/01/01
    saving-card                           $-105
    snacks                       95 EUR @@ $100
    Equity:Unbalanced                      $105    ; absence of $5 here triggers strange behavior
    Equity:Unbalanced           -95 EUR @@ $100    ; absence of $5 here triggers strange behavior
Collaborator

ony commented Jan 11, 2017

Though journal now is valid I've got strange output for journal

2016/1/1
    saving-card  $-105
    snacks  95 EUR @@ $100
    Equity:Unbalanced  ; absence of $5 here triggers strange behavior
2016/01/01
    saving-card                 $-105
    snacks             95 EUR @@ $100
    Equity:Unbalanced            $105    ; absence of $5 here triggers strange behavior
    Equity:Unbalanced           $-100    ; absence of $5 here triggers strange behavior

I would understand if the output would look like:

2016/01/01
    saving-card                           $-105
    snacks                       95 EUR @@ $100
    Equity:Unbalanced                      $105    ; absence of $5 here triggers strange behavior
    Equity:Unbalanced           -95 EUR @@ $100    ; absence of $5 here triggers strange behavior
@simonmichael

This comment has been minimized.

Show comment
Hide comment
@simonmichael

simonmichael Jan 11, 2017

Owner

I see, it looks like the balancing amount inferring function is missing an opportunity to combine same-commodity amounts. It's normal for the inferred balancing amount to be converted to the price's commodity ($) by the way, hledger always does that for similarity with Ledger/convenience reasons I don't fully recall.

Owner

simonmichael commented Jan 11, 2017

I see, it looks like the balancing amount inferring function is missing an opportunity to combine same-commodity amounts. It's normal for the inferred balancing amount to be converted to the price's commodity ($) by the way, hledger always does that for similarity with Ledger/convenience reasons I don't fully recall.

ony added a commit to ony/hledger that referenced this issue Jan 11, 2017

Normalize amount in infereBalancingAmount
This fixes issue exposed by a fix for simonmichael#465

simonmichael added a commit that referenced this issue Jan 11, 2017

Normalize amount in infereBalancingAmount (#469)
This fixes issue exposed by a fix for #465
@rmoehn

This comment has been minimized.

Show comment
Hide comment
@rmoehn

rmoehn Jan 21, 2017

Looks like it works for me now! Thanks!

rmoehn commented Jan 21, 2017

Looks like it works for me now! Thanks!

mstksg added a commit to mstksg/hledger that referenced this issue Feb 3, 2017

print: omit the last posting amount, as we used to simonmichael#465, s…
…imonmichael#442

This avoids printing invalid journal format for entries where an implicit amount has multiple commodities.

mstksg added a commit to mstksg/hledger that referenced this issue Feb 3, 2017

mstksg added a commit to mstksg/hledger that referenced this issue Feb 3, 2017

Ensure showTransaction produce a valid journal (simonmichael#466)
- Make output of print to be a valid journal
- Partially reverts 419f5f2

Fixes simonmichael#465

mstksg added a commit to mstksg/hledger that referenced this issue Feb 3, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment