wolph / mt940

A library to parse MT940 files and returns smart Python collections for statistics and manipulation.
https://mt940.readthedocs.org/en/latest/
BSD 3-Clause "New" or "Revised" License
94 stars 50 forks source link

Transaction details not parsed for GVC 051, 174 #49

Closed sbi closed 6 years ago

sbi commented 7 years ago

transaction_details_post_processor does not parse transation details for GVC 051 ("Gutschrift Auslandszahlungsverkehr") and 174 ("Sammeleinziehung"). Can I ammend the gvcs list?

wolph commented 7 years ago

Yes, of course :)

I accept any pull request that doesn't break the current parser and is tested.

FelixSchwarz commented 7 years ago

I'd like to add GVC codes for:

@Wolph: Is there a past commit/pull request which you consider a good "template" so I can easily see which tests are expected?

FelixSchwarz commented 7 years ago

Btw: Maybe it is even better to remove testing for specific gvcs? There is commit 784f3025cda5a6411ad32805934aacbc4287baf1 in a fork of mt940 which does exactly this...

I'm asking this because I also need 106 + 107 but basically all I have to do is to add these numbers to the list of gvcs...

wolph commented 7 years ago

Yes, I agree. It might be better to drop that requirement.

As for the pull requests, the simplest way to test is to install tox and have it run the tests automatically:

pip install tox
tox

Note that Travis automatically tests all pull requests anyhow but tox is faster for local development: https://travis-ci.org/WoLpH/mt940/pull_requests

FelixSchwarz commented 7 years ago

So you agree that just dropping the check is likely the best way forward? In that case you might want to cherry-pick https://github.com/mecodia/mt940/commit/784f3025cda5a6411ad32805934aacbc4287baf1

My question about pull requests was more about which kind of tests are expected for this – do you need only unit tests or integration tests based on full "MT 940" data? How to deal with the potentially big amount of "noise" (unrelated data - how to document the important parts?) in such an integration test? How does the testing strategy work in general? In the past I noticed that looking at a few "good examples" helps me asking the right questions. However these questions can wait as it seems there is a simpler path forward...

wolph commented 7 years ago

I'm not sure why that code has disappeared but cherry-picking seems like a good solution.

For code that's easily tested I prefer to go with regular unit tests such as these: https://github.com/WoLpH/mt940/blob/develop/tests/test_processors.py

Beyond that I've tried got gather as much mt940 files as possible to do integration tests which are run here: https://github.com/WoLpH/mt940/blob/de75e5a4826b9a5ee2bcd10ade18317baa298b2f/tests/test_sta_parsing.py#L77-L106

To generate your own yml files from the mta files, temporarily uncomment this line: https://github.com/WoLpH/mt940/blob/de75e5a4826b9a5ee2bcd10ade18317baa298b2f/tests/test_sta_parsing.py#L82

wolph commented 7 years ago

I've merged that change in so the strict checking is now disabled :)

Note that until the changes are merged you can easily monkeypatch the behaviour:

from mt940 import processors

processors.DETAIL_KEYS['20'] = 'purpose'
FelixSchwarz commented 7 years ago

Thanks a lot - latest git fixes the problem me.

@sbi: Maybe we can close this ticket now?

wolph commented 6 years ago

Closing the ticket due to inactivity and because @FelixSchwarz confirmed it works :)

sbi commented 6 years ago

Oops, sorry for my late reply, that cherry picked change is what I was looking for, too. I applied it locally and it's working fine :-)