railslove / cmxl

your friendly MT940 SWIFT file parser for bank statements
http://railslove.com
MIT License
46 stars 25 forks source link

Added strip_headers conifguration option to remove SWIFT header data before parsing #13

Closed bumi closed 5 years ago

bumi commented 7 years ago

some MT940 files come with additional header information. Currently parsing failes when these headers are present. This options tries to clean up the file and remove the headers before parsing as mostly this information is not needed. By default this option is disabled and should only be enabled when these header files are present.

As a further improvement we should support parsing of the header data aswell.

bumi commented 7 years ago

related: #9

@kangguru @namxam what do you think?

hmac commented 5 years ago

This would be a really useful feature for us! Any chance we can get this merged?

bumi commented 5 years ago

@hmac thanks for the bump. @Uepsilon what do you think can we merge this? It is not changing the current behaviour and simply adds the option to strip any line that does NOT start with : at the beginning and the end of the file. parsing those somehow would be nicer but in most cases those are not needed.

Uepsilon commented 5 years ago

@bumi code looks fine to me. yet this feature is something i'd rather see in the code utilising our gem. this parser should not be responsible for finding what to parse but parse what is provided and throw an error if it's not parseable.

bumi commented 5 years ago

hmm... I see your point. but I can see how it is useful... some banks have that header and in my code I just want to pass whatever the bank provides to the parser without any pre-parsing of the data.

Uepsilon commented 5 years ago

It sure is useful. Go ahead, do it. Let's just be careful not to overload this gem with features not necessarily aligned with the core task

bumi commented 5 years ago

yep, I totally agree and I am not sure about this... so I hesitate and appreciate your opinion!

@hmac what do you think?

hmac commented 5 years ago

I think in the long run, it'd definitely be nicer if we properly parsed these headers. However in the absence of that, I'd rather that they get stripped out here, close to the rest of the parsing logic, than in surrounding downstream code. Catching cmxl errors and trying to work out if they're caused by the presence of swift headers or by an actual parsing issue doesn't sound like a very robust approach.

@Uepsilon I appreciate that you don't want to overload the gem - I think that's a really valid concern. Would you be happier if we instead attempted to parse these headers?

Uepsilon commented 5 years ago

The swift headers have nothing to do with MT940. The data should always be included in the {4:...}-block. At best dumping the headers into this parser should produce a non-parsable error.

Since the primary usage of this gem is related to SWIFT, I could agree to adding the stripping of said headers. But at best the stripping would happen within a separate gem which parses the swift message and utilises this gem if it detects MT940 blocks in the SWIFT text block.

Uepsilon commented 5 years ago

but for convenience sake i'd say merge it, since the change aren't really that big and it's fully downward-compatible and should not have side effects unless explicitly activated