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 support for LedgerBalance. Better data type conversion. Unit Tests #4

Merged
merged 1 commit into from
Sep 8, 2019

Conversation

DashNY
Copy link
Contributor

@DashNY DashNY commented Aug 30, 2019

  • Added support for LEDGERBAL tag
  • Refactored some logic for better type conversion
  • Refactored the datetime parsing, made it clearer. Let me know what you think.
  • Added unit tests

@DashNY DashNY force-pushed the feature-ledgerbal branch from 30bf1f4 to 3d009b7 Compare August 30, 2019 21:12
Copy link
Owner

@eramella eramella left a comment

Choose a reason for hiding this comment

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

Thank you for your PR Alec. Well done. We will need to work on some more tests, now that you add them....:):)
One little thing I would like you to fix. See my notes on ConvertQfxType method.

I also noticed a problem in my code. Some .qfx or .qbo file have different type of transactions but same transaction properties as we have (i.e. investment transaction 'INVTRAN' with nested 'FITID'), this will return a type of PropertyTransaction but the cycle has a null _currentTransaction which throws. We need to check to make sure currentTransaction is not null before calling SetValue(). I will fix it as soon as I have a little time.
Thank you again!

QFXparser/QFXparser.cs Outdated Show resolved Hide resolved
@DashNY
Copy link
Contributor Author

DashNY commented Sep 5, 2019

I'm wondering if there is a way to make it run tests as part of each PR. Any idea?

@eramella
Copy link
Owner

eramella commented Sep 5, 2019 via email

@DashNY
Copy link
Contributor Author

DashNY commented Sep 5, 2019

Made the changes you requested. When you have a moment, please merge the PR and publish a nuget.

Not for this PR, but perhaps to improve/unclutter the overall parsing logic, there are parsers out there. The industry standard is ANTLR but it's super bulky (written in Java etc)
I see a better option is Sprache
https://github.com/sprache/Sprache
There is even an example of parsing XML documents.
https://github.com/sprache/Sprache/blob/master/samples/XmlExample/Program.cs
I understand QFX is some weird partial XML format. Still it should be easy enough to implement QFX parser using something like Sprache.

@DashNY
Copy link
Contributor Author

DashNY commented Sep 5, 2019

On another note, I don't have any examples of those .qfx / .ofx files.
If you do, please include them to the unit testing project next to test.qfx.
I'd like to see what they look like. Thanks.

@eramella
Copy link
Owner

eramella commented Sep 5, 2019

Thank you Alec, I will work on the code during the weekend, merge and push to Nuget.
I really like that Sprache approach! Awesome find. Looks great. The only thing is that this was my pet project to get my feet wet into parsing. Obviously it is not perfect and not very extensible, but I would really like to keep it also in an effort not to depend on other projects, at least for now.
I will include the .qfx file that throws the error. It is a statement from a financial broker but I would need to sanitize the content first. It has transactions with buy and sell flags and others goodies...:).
I started this project to mainly parse credit card transactions that I needed for another project. After long researching for libraries in this fields I decided to build it myself. Obviously this system can be used in other cases as well. Thank you again for expanding its capabilities.

@eramella eramella merged commit d58f67c into eramella:master Sep 8, 2019
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