search

egh / ledger-autosync

Synchronize your ledger-cli files with your bank.
GNU General Public License v3.0
273 stars 42 forks source link

Bugfix to include fees #110

Closed emin63 closed 4 years ago

emin63 commented 4 years ago

This is a simple PR with a bugfix to include fees. See https://github.com/egh/ledger-autosync/issues/109.

Basically, in the case where there are fees in a stock trade, you need to adjust the total by the amount of the fees and also include a posting for the fees. If you don't do that, then your accounts will be wrong (at least for OFX files from Vanguard).

Note: I tried to run the unit tests you have but couldn't get them working. I think things were failing at import ledger. I installed hledger and have ledger-cli installed but it looks like I need some other python package for ledger but that was not in requirements.txt.

~If you provide docs on how to run tests, I am happy to try and write a test for this.~

I'm still not quite sure how to run your tests but I added a new test for the proposed fix and that seems to work.

egh commented 4 years ago

Looks good to me! Thank you.

Yes, the tests are a little old and crufty. I'd like to rewrite with pytest. Usually nosetests -a generic -a ledger should exercise them. There is an (old) python-ledger library that is used if present, but it's hard to build and complex.