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

Negating amount breaks parser for negative amounts #524

Closed
joshkehn opened this Issue Mar 24, 2017 · 4 comments

Comments

Projects
None yet
2 participants
@joshkehn

joshkehn commented Mar 24, 2017

Reproducing

Specific trace:

hledger: error: could not parse "$--5000.00" as an amount
the CSV record is:       "2017/01/04", "Merchant Credit", "-5000.00"
the amount rule is:      -%payment
the currency rule is:    $
the default-currency is: unspecified
the parse error is:      ParseError {errorPos = SourcePos {sourceName = "", sourceLine = Pos 1, sourceColumn = Pos 3} :| [], errorUnexpected = fromList [Tokens ('-' :| "")], errorExpected = fromList [Tokens (',' :| ""),Tokens ('.' :| ""),Label ('d' :| "igit")], errorCustom = fromList []}
you may need to change your amount or currency rules, or change your skip rule

Attached the rules bank.csv.rules.txt and the CSV bank.csv.txt which cause this.

Why this matters

First, it's in an example rules.

Second, currently American Express outputs amounts as “inflow” (positive) amounts in—some—CSV exports. It's a liability account, it has a credit balance, these are credits to the liability and a debit to the expense account.

A possible user solution is to flip account1 and account2 and let some other logic in hledger figure out the balance to credit the liability. This means a common set of rules can't be used since another CSV might have “outflow” (negative) amounts. In that case, account1 and account2 for common rules would be reversed.

@simonmichael

This comment has been minimized.

Show comment
Hide comment
@simonmichael

simonmichael Mar 25, 2017

Owner

You're right, we don't parse a double minus sign. The CSV reader also recognises parentheses as negative, so it's better to use those for sign-flipping: amount (%payment). I see several instances of -% in examples/csv-rules/*.

I suppose it's a good idea to change the amount parser in JournalReader.hs to allow a double minus sign. Would you be willing to give that a try ? Support available in #hledger if you need.

Owner

simonmichael commented Mar 25, 2017

You're right, we don't parse a double minus sign. The CSV reader also recognises parentheses as negative, so it's better to use those for sign-flipping: amount (%payment). I see several instances of -% in examples/csv-rules/*.

I suppose it's a good idea to change the amount parser in JournalReader.hs to allow a double minus sign. Would you be willing to give that a try ? Support available in #hledger if you need.

@simonmichael

This comment has been minimized.

Show comment
Hide comment
@simonmichael

simonmichael Mar 25, 2017

Owner

So these would be equivalent to $1: --$1, $--1, and -$-1 too ? On second thought, this is strange syntax that might be better confined to the CSV reader only.

Owner

simonmichael commented Mar 25, 2017

So these would be equivalent to $1: --$1, $--1, and -$-1 too ? On second thought, this is strange syntax that might be better confined to the CSV reader only.

@simonmichael simonmichael added the easy? label Mar 25, 2017

@joshkehn

This comment has been minimized.

Show comment
Hide comment
@joshkehn

joshkehn Mar 26, 2017

@simonmichael Yes, idea would be --$1 == $1 == -$-1. However, I find --$1 to be a bit confusing. I much prefer the amount (%payment) idea as explicit negation.

That said, I did take a look at the CSV parsing code in the area you indicated. It seems like the possible solution would be to expand negateIfParenthesised and negate the amount if it starts with a -. This would mean negateStr needs to understand -<currency?>-<amount> is <currency?><amount>.

joshkehn commented Mar 26, 2017

@simonmichael Yes, idea would be --$1 == $1 == -$-1. However, I find --$1 to be a bit confusing. I much prefer the amount (%payment) idea as explicit negation.

That said, I did take a look at the CSV parsing code in the area you indicated. It seems like the possible solution would be to expand negateIfParenthesised and negate the amount if it starts with a -. This would mean negateStr needs to understand -<currency?>-<amount> is <currency?><amount>.

Sebatyne added a commit to Sebatyne/hledger that referenced this issue May 13, 2017

CsvReader: simplify '-' signs if there exist more than one
It prevents hledger to crash later, as it fails to read
amount strings containing more than one '-'

Fix simonmichael#524

Sebatyne added a commit to Sebatyne/hledger that referenced this issue May 13, 2017

CsvReader: simplify '-' signs if there exist more than one
It prevents hledger to crash later, as it fails to read
amount strings containing more than one '-'

Fix simonmichael#524

simonmichael added a commit that referenced this issue May 14, 2017

CsvReader: simplify '-' signs if there exist more than one (#548)
It prevents hledger to crash later, as it fails to read
amount strings containing more than one '-'

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