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

"balance" parser for csv reader #537

Merged
merged 2 commits into from Apr 14, 2017

Conversation

Projects
None yet
2 participants
@adept
Collaborator

adept commented Apr 13, 2017

Ability to add balance assertions in csv reader

@simonmichael

This comment has been minimized.

Show comment
Hide comment
@simonmichael

simonmichael Apr 14, 2017

Owner

Nice! I ask myself how much benefit this brings, but I currently put paypal's balance in a transaction comment for troubleshooting/informational purposes, so I guess why not check it as well, given that the performance impact seems quite small.

But as you mentioned in IRC, ordering issues might be a problem. We might get multiple same-day transactions, and we can't rely on the ordering of same-day CSV records. We might also see multiple same-account postings in a transaction (if not now, then in future). How easy/efficient would it be to omit the assertions in just these cases ?

Owner

simonmichael commented Apr 14, 2017

Nice! I ask myself how much benefit this brings, but I currently put paypal's balance in a transaction comment for troubleshooting/informational purposes, so I guess why not check it as well, given that the performance impact seems quite small.

But as you mentioned in IRC, ordering issues might be a problem. We might get multiple same-day transactions, and we can't rely on the ordering of same-day CSV records. We might also see multiple same-account postings in a transaction (if not now, then in future). How easy/efficient would it be to omit the assertions in just these cases ?

@simonmichael

This comment has been minimized.

Show comment
Hide comment
@simonmichael

simonmichael Apr 14, 2017

Owner

PS I'd guess trying to re-order things automatically to make assertions work would bring headaches. We don't want to risk changing the meaning of the data.

Owner

simonmichael commented Apr 14, 2017

PS I'd guess trying to re-order things automatically to make assertions work would bring headaches. We don't want to risk changing the meaning of the data.

@adept

This comment has been minimized.

Show comment
Hide comment
@adept

adept Apr 14, 2017

Collaborator

I've played a bit more with this patch, and I am liking it more and more.

It is highly unlikely that within singe CSV file that has balance there would be out-of-order transactions. In only encounetered issues with multiple current accounts in a single bank, where each account have its own CSV file and transfers between account are therefore present in both.

Previously i've been dropping one transfer transaction out of two, pretty much at random, and this is what played havoc with balance assertions. Once I kept both of them, writing a rule so that one transfer is getting imported as "account A -> special account for transfers" and second transfer is "special account for transfers -> account B", my balances got in order and every assertion checked out.

I no longer believe that my reordering idea is necessary at all, and you are right - it is unnecessart complicated.

Collaborator

adept commented Apr 14, 2017

I've played a bit more with this patch, and I am liking it more and more.

It is highly unlikely that within singe CSV file that has balance there would be out-of-order transactions. In only encounetered issues with multiple current accounts in a single bank, where each account have its own CSV file and transfers between account are therefore present in both.

Previously i've been dropping one transfer transaction out of two, pretty much at random, and this is what played havoc with balance assertions. Once I kept both of them, writing a rule so that one transfer is getting imported as "account A -> special account for transfers" and second transfer is "special account for transfers -> account B", my balances got in order and every assertion checked out.

I no longer believe that my reordering idea is necessary at all, and you are right - it is unnecessart complicated.

@simonmichael

This comment has been minimized.

Show comment
Hide comment
@simonmichael

simonmichael Apr 14, 2017

Owner

Ok, let's merge this and people can try it. In case of trouble they can not use it or do manual fixups. I think it just needs some docs.

Owner

simonmichael commented Apr 14, 2017

Ok, let's merge this and people can try it. In case of trouble they can not use it or do manual fixups. I think it just needs some docs.

@adept

This comment has been minimized.

Show comment
Hide comment
@adept

adept Apr 14, 2017

Collaborator

I've added a bit of documentation

Collaborator

adept commented Apr 14, 2017

I've added a bit of documentation

@simonmichael simonmichael merged commit 451f9d7 into simonmichael:master Apr 14, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@simonmichael

This comment has been minimized.

Show comment
Hide comment
@simonmichael

simonmichael Apr 14, 2017

Owner

Looks good, thanks!

Owner

simonmichael commented Apr 14, 2017

Looks good, thanks!

@simonmichael

This comment has been minimized.

Show comment
Hide comment
@simonmichael

simonmichael Apr 17, 2017

Owner

Some issues noticed recently:

  • in several rules files I already had a balance field, which I ignored or included as a comment. After upgrading hledger from 1.2 to HEAD those conversions all broke, eg because the bank sometimes leaves the balance field blank.

  • balance assertions aren't valid in the converted journal, because the CSV was just a recent subset of transactions. If you want to run hledger against the converted journal, you have to remember to add -I/--ignore-assertions.

Owner

simonmichael commented Apr 17, 2017

Some issues noticed recently:

  • in several rules files I already had a balance field, which I ignored or included as a comment. After upgrading hledger from 1.2 to HEAD those conversions all broke, eg because the bank sometimes leaves the balance field blank.

  • balance assertions aren't valid in the converted journal, because the CSV was just a recent subset of transactions. If you want to run hledger against the converted journal, you have to remember to add -I/--ignore-assertions.

@simonmichael

This comment has been minimized.

Show comment
Hide comment
@simonmichael

simonmichael Apr 17, 2017

Owner

PS so the "If you assign empty string to it, no assertion will be generated" case mentioned in the docs, doesn't seem to be working here.

Owner

simonmichael commented Apr 17, 2017

PS so the "If you assign empty string to it, no assertion will be generated" case mentioned in the docs, doesn't seem to be working here.

@adept

This comment has been minimized.

Show comment
Hide comment
@adept

adept Apr 18, 2017

Collaborator
Collaborator

adept commented Apr 18, 2017

@adept

This comment has been minimized.

Show comment
Hide comment
@adept

adept Apr 18, 2017

Collaborator

To be precise, I previously tested that this will work:

if
match for transaction that needs to have balance assertion excluded
balance

But I failed to test that empty or all-space value for balance will work. #539 should address this.

Collaborator

adept commented Apr 18, 2017

To be precise, I previously tested that this will work:

if
match for transaction that needs to have balance assertion excluded
balance

But I failed to test that empty or all-space value for balance will work. #539 should address this.

@simonmichael

This comment has been minimized.

Show comment
Hide comment
@simonmichael

simonmichael Apr 18, 2017

Owner

Thanks!

Owner

simonmichael commented Apr 18, 2017

Thanks!

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