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

Parse all aggregations #85

Merged
merged 2 commits into from
Feb 17, 2016
Merged

Conversation

egh
Copy link
Contributor

@egh egh commented Sep 10, 2015

I was missing transactions when using ledger-autosync. It turns out they were <TRANSFER> aggregations. This modifies ofxparse to parse all aggregation types (as defined in 2.1.1 of the spec (http://www.ofx.net/DownloadPage/Downloads.aspx)). It also changes the type attribute of the InvestmentTransaction to be a string instead of the less useful number.

Thanks!

- Investments transactions can have many types; parse them all.
- Change type attribute on InvestmentTransaction to a string
Tests <TRANSACTION> aggregate type, among other things.
@egh
Copy link
Contributor Author

egh commented Sep 12, 2015

Related: #74 but this a little more comprehensive, and also includes a behavior change. Thank you!

@egh
Copy link
Contributor Author

egh commented Oct 12, 2015

If you'd prefer, I can switch back to the enum-style type. I notice you are using that pattern elsewhere.

@jseutter jseutter merged commit 599396b into jseutter:master Feb 17, 2016
@jseutter
Copy link
Owner

I'm fine with the pattern you used. Thank you for the fixes (and tests)!

@jseutter
Copy link
Owner

I also added you to AUTHORS

@egh
Copy link
Contributor Author

egh commented Feb 17, 2016

Thank you!

@egh egh mentioned this pull request Mar 14, 2016
@egh egh deleted the fix/parse_all_aggregations branch July 19, 2020 23:30
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.

None yet

2 participants