Skip to content
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

Add dataparser for handelsbanken / BEC banks #23

Merged
merged 3 commits into from
Feb 5, 2018
Merged

Add dataparser for handelsbanken / BEC banks #23

merged 3 commits into from
Feb 5, 2018

Conversation

larsbaunwall
Copy link
Contributor

Add support for Handelsbanken / BEC export in the format [Date;Text;Amount;Saldo;Dummy]

@Laumania
Copy link
Owner

Laumania commented Dec 1, 2017

Nice and thanks! Always good with PR's - I'll look at it later and merge it.

@Laumania
Copy link
Owner

Laumania commented Dec 7, 2017

@larsbaunwall Sorry, haven't merged yet, as I haven't had the time - busy at work - I will look at it soon how ever :)

Again, thanks for you pull request.

@Laumania
Copy link
Owner

Laumania commented Dec 8, 2017

Hi @larsbaunwall
Again, sorry for the delay. I looked at your changes and let me say first, nicely done - you keep indenting stuff as the rest of the project - the OCD approves :P

However, could I ask you to add a Unittest for your new DataParser too?

Copy link
Owner

@Laumania Laumania left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Never tried this "Review" thing before to be honest. Seems pretty smart :)

}

[DelimitedRecord(";"), IgnoreEmptyLines(), IgnoreFirst()]
private class SparNordEntry
Copy link
Owner

@Laumania Laumania Dec 8, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@larsbaunwall Guess you missed renaming 'SparNordEntry'? I know it's private and all, but the name is miss leading.

@Laumania
Copy link
Owner

@larsbaunwall Hi Lars, did you have time to look at my comments?
I really want to have a Unittest added with a sample of the BEC file format, to make sure it doesn't break the logic behind finding the right parser for the uploaded file.

@Laumania
Copy link
Owner

@larsbaunwall I'm not sure if you don't see these messages or you just don't have time to look into this. Can you please reply, if you get these and just haven't had time to look into it?

I would very much like to have this PR merged in :)

@larsbaunwall
Copy link
Contributor Author

larsbaunwall commented Jan 16, 2018

Hi Mads.
You just saw your messages - have been occupied elsewhere for the last month or so.
I will look into your comments as soon as possible 👍

@larsbaunwall
Copy link
Contributor Author

Just committed the changes - hope you like them :)

@Laumania
Copy link
Owner

@larsbaunwall No problem, no rush - just wanted to make sure you got the notification, so at least that wasn't the reason why :)

Very nice, I'll take a look at your changes soon - thanks for your pull request once again!

@Laumania Laumania merged commit a0bad9f into Laumania:master Feb 5, 2018
@Laumania
Copy link
Owner

Laumania commented Feb 5, 2018

@larsbaunwall Looked good! I see 2 unittests fails, but they do that in master too, so it's not your fault. I'll look at it in the master branch.

Thank you so much for your Pull Request - keep 'em coming :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants