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

Wrong calculation due to incorrect expression parsing with subtraction #2001

Closed
dslad opened this issue Feb 8, 2021 · 11 comments
Closed

Wrong calculation due to incorrect expression parsing with subtraction #2001

dslad opened this issue Feb 8, 2021 · 11 comments
Labels
bug Something isn't working P1 Highest priority
Milestone

Comments

@dslad
Copy link

dslad commented Feb 8, 2021

This works as expected (with a space between '3' and '-')

2020/01/01 Works
    Assets:Cash    (£10*(3 -1))
    Equity

But this transaction is silently ignored, I guess because the expression incorrectly evaluates to zero

2020/01/01 Doesn't work
    Assets:Cash    (£10*(3-1))
    Equity
@tbm tbm added the bug Something isn't working label Feb 8, 2021
@tbm
Copy link
Contributor

tbm commented Feb 8, 2021

I guess because the expression incorrectly evaluates to zero

I don't know why, but you are absolutely right.

~/tmp/src/ledger/ledger -f  d reg
~/tmp/src/ledger/ledger -f  d reg --empty
2020-01-01 Doesn't work     Assets:Cash                                        0                        0
                            Equity                                             0                        0

Debug:

   25ms  [DEBUG] num prec = 385
   25ms  [DEBUG] den prec = 385
   25ms  [DEBUG] mpfr_print = 0 (precision 0, zeros_prec 0)
   25ms  [DEBUG] £0

@tbm
Copy link
Contributor

tbm commented Feb 8, 2021

--debug expr.calc says:

    1ms  [DEBUG] Calculating: (£10*(3-1))
    1ms  [DEBUG]   (£10 * 0) => ...
    1ms  [DEBUG] .  0 => ...
    1ms  [DEBUG] .  0 => 0
    2ms  [DEBUG] .  £10 => ...
    2ms  [DEBUG] .  £10 => £10
    2ms  [DEBUG]   (£10 * 0) => £0

@tbm tbm changed the title Spaces in expression parsing Wrong calculation due to incorrect expression parsing Feb 8, 2021
@tbm
Copy link
Contributor

tbm commented Feb 8, 2021

Even simpler testcase:

2020/01/01 Doesn't work
    Assets:Cash    (3-1)
    Equity

debug:

    3ms  [DEBUG] Calculating: (3-1)
    3ms  [DEBUG]   0 => ...
    3ms  [DEBUG]   0 => 0

@tbm
Copy link
Contributor

tbm commented Feb 8, 2021

These all work fine:

2020/01/01 works
    Assets:Cash    (3*1)
    Equity

2020/01/01 works
    Assets:Cash    (3/1)
    Equity

2020/01/01 works
    Assets:Cash    (3+1)
    Equity

@tbm tbm changed the title Wrong calculation due to incorrect expression parsing Wrong calculation due to incorrect expression parsing with subtraction Feb 8, 2021
@tbm
Copy link
Contributor

tbm commented Feb 8, 2021

This was actually reported before, see #1809

@tbm tbm added the P1 Highest priority label Feb 8, 2021
@tbm tbm added this to the 3.3.x milestone Feb 8, 2021
@enderw88
Copy link
Member

enderw88 commented Feb 8, 2021 via email

@tbm
Copy link
Contributor

tbm commented Feb 8, 2021

none of those have a commodity assigned. Does subtraction work if it does
have a commodity assigned?

yes, but imho that's not the point since a) value expressions support math, b) the original example had a commodity somewhere in the expression, and c) ledger doesn't require commodities.

@enderw88
Copy link
Member

enderw88 commented Feb 8, 2021 via email

@dslad
Copy link
Author

dslad commented Feb 8, 2021

:) Seems to still recognise minus sign at the beginning of a quantity - it's ones in the middle that were causing problems.

oaf-edwin added a commit to oaf-edwin/ledger that referenced this issue Apr 11, 2021
An amount may have a (single) leading minus sign, but none after that.
Bug ledger#2001 (and ledger#1809).
jwiegley pushed a commit that referenced this issue May 2, 2021
An amount may have a (single) leading minus sign, but none after that.
Bug #2001 (and #1809).
@tbm
Copy link
Contributor

tbm commented Aug 4, 2021

I think this can be closed since PR #2027 got merged. Right?

@tbm
Copy link
Contributor

tbm commented Aug 4, 2021

Fixed in commit 4ba80c3

@tbm tbm closed this as completed Aug 4, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working P1 Highest priority
Projects
None yet
Development

No branches or pull requests

3 participants