-
Notifications
You must be signed in to change notification settings - Fork 48
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
Fix Schwab parsing #459
Fix Schwab parsing #459
Conversation
8549890
to
a9931ca
Compare
Can you try again? I downloaded my transactions and it looks like Quantity and Price columns are swapped. |
Ah yes. You are right. Not sure if this is temporary or permanent. I guess the proper fix would be to determine the column purpose based on the header. |
I agree. If you add it I can test it on my statement. You can do something like this:
|
4ab38be
to
74c02a8
Compare
@vmartinv I have made the changes and tested using Individual Transaction History and Equity Awad Transaction History file. Please test and if you have permission, merge the changes. |
I think it looks good but can you also add a validation to make sure the columns are as expected? Something like this:
Otherwise a format change might go unnoticed. |
I did not do this because I didn't want the script to break if an additional column is added that we do not use. However, I also see the reason why we need to check for headers. So, I have taken an intermediate ground and updated the code to ensure that the headers that we are using are present in the transactions file. We do not care if there are additional headers in the transactions file that we are not using. |
6fb4a99
to
c413be6
Compare
c413be6
to
55799c1
Compare
Sounds good to me. I can confirm this PR works as expected, it parsed my latest Schwab files and generated the report correctly. Can you confirm @KapJI ? |
Looks good, thank you! And thanks @vmartinv for help with reviewing this. |
Updated parser logic for Schwab
Fixes #451
Fixes #457
Fixes #464