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

[RFC][WIP] Add importer for Schwab brokerage CSV exports. #77

Merged
merged 15 commits into from
Dec 17, 2020

Conversation

carljm
Copy link
Collaborator

@carljm carljm commented Dec 6, 2020

Schwab recently (in October) discontinued their OFX download support, so I wrote an importer for their CSV exports instead. It mostly tries to emulate the transactions that would have previously been generated by their OFX, and at least for my past couple months' Schwab history, it mostly seems to succeed.

I think this needs some more work before it's mergeable, but wanted to post it for early review and comments. Assuming it looks generally OK and you'd be open to merging it, I can look into taking care of the remaining items. It at least needs tests and docs. There are also some TODO items in the code; I'd welcome your feedback on which of those you'd consider as prereqs for merge.

  • tests (I have some locally but they currently depend on my entire Schwab history as test data, so that needs cleaning up before I can make them public)
  • docs (module docstring explaining the importer's functionality and how to configure it)

@coveralls
Copy link

coveralls commented Dec 6, 2020

Pull Request Test Coverage Report for Build 197

  • 497 of 526 (94.49%) changed or added relevant lines in 4 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+1.6%) to 68.713%

Changes Missing Coverage Covered Lines Changed/Added Lines %
beancount_import/source/schwab_csv.py 485 514 94.36%
Totals Coverage Status
Change from base Build 185: 1.6%
Covered Lines: 5112
Relevant Lines: 7202

💛 - Coveralls

@coveralls
Copy link

Pull Request Test Coverage Report for Build 186

  • 2 of 485 (0.41%) changed or added relevant lines in 3 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-67.07%) to 0.0%

Changes Missing Coverage Covered Lines Changed/Added Lines %
beancount_import/source/schwab_csv.py 0 483 0.0%
Totals Coverage Status
Change from base Build 185: -67.07%
Covered Lines: 0
Relevant Lines: 0

💛 - Coveralls

@Zburatorul
Copy link
Collaborator

What's the config dict meant to look like?

dict(
              module="beancount_import.source.schwab_csv",
              transaction_csv_filenames=glob.glob(os.path.join(data_dir, "schwab/csv/*Transactions*.csv")),
              position_csv_filenames=glob.glob(os.path.join(data_dir, "schwab/csv/*Positions*.csv"))
          )

@carljm
Copy link
Collaborator Author

carljm commented Dec 9, 2020

Yup pretty much like that! I download positions and transactions into separate subdirs which simplifies the globs, but that shouldn't make any difference.

@jbms
Copy link
Owner

jbms commented Dec 9, 2020

Another option you could consider is to convert the CSV statements into OFX format, e.g. as in this pull request:

#58

using ofxstatement (https://github.com/kedder/ofxstatement).

That way your converter would not be tied to beancount-import at all and could also be used by people that are using different accounting software.

@jbms
Copy link
Owner

jbms commented Dec 9, 2020

From looking at your code, I see that the transactions CSV file unfortunately does not seem to include a unique id column, which would make converting to OFX problematic. So I think that indeed creating a separate importer as you have done may be the best way to go.

@carljm
Copy link
Collaborator Author

carljm commented Dec 9, 2020

Yeah conversion to OFX was my initial plan (didn't know about ofxstatement though, that's cool!) and I ran into the lack of unique ID.

@Zburatorul
Copy link
Collaborator

Yup pretty much like that! I download positions and transactions into separate subdirs which simplifies the globs, but that shouldn't make any difference.
Thanks.

There are a few action types not yet implemented: Buy_to_Close, Sell_to_Close, Margin_Interest, Expired, ADR Mgmt Fee, Qualified Dividend, Foreign Tax Paid.

@carljm
Copy link
Collaborator Author

carljm commented Dec 10, 2020

Indeed. Some of those don't currently occur in my personal Schwab history. So either we will need to merge this in an incomplete state and allow others to supply PRs for missing bits, or someone will need to send me sample CSV rows for those transaction types so I can implement them.

@carljm
Copy link
Collaborator Author

carljm commented Dec 16, 2020

Ok, I added tests and docs and fixed auto-creation of needed per-security sub-accounts. I think the only remaining issue here is getting more coverage of Schwab "actions" (transaction types). Not sure I'll be able to get that complete (AFAIK there's no reference for what "complete" even is, all I can go by is what appears in my own transaction history and anyone else who tries this), but I'll add the things I'm aware of at least.

@carljm
Copy link
Collaborator Author

carljm commented Dec 17, 2020

@jbms I think this branch is merge-ready, would you be willing to give it a review?

I know it doesn't support all of the transaction types that can be present in a Schwab CSV, but since it's very hard to add these without seeing examples of them in a real transaction history, I think it will be necessary for various users to just fill out the completeness over time. I'm happy to review pull requests against this importer going forward.

Travis CI is not in a working state, but I can confirm that tox passes cleanly on the latest commit here; that runs mypy and pytest on both py37 and py38.

@jbms
Copy link
Owner

jbms commented Dec 17, 2020

Looks great overall and thanks for including tests, I just had a couple small comments.

@carljm
Copy link
Collaborator Author

carljm commented Dec 17, 2020

Looks great overall and thanks for including tests, I just had a couple small comments.

Thanks for reviewing! I don't see any code comments though; did you forget to submit your review after adding comments?

@carljm
Copy link
Collaborator Author

carljm commented Dec 17, 2020

Added support for a few more transaction types sent to me by another user. Tox still passing clean.

InvalidSourceReference(extra, entry_posting_pairs)
)

for balance_entry in balance_entries:
Copy link
Owner

Choose a reason for hiding this comment

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

Beancount-import itself removes imported price and balance entries that are already present in the journal, so you can skip this here.


class SchwabSource(DescriptionBasedSource):
name = "schwab"

Copy link
Owner

Choose a reason for hiding this comment

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

I'd recommend overriding get_example_key_value_pairs to also include schwab_action.

@jbms
Copy link
Owner

jbms commented Dec 17, 2020

Yes, indeed I forgot to "submit review" --- that was non-obvious from the github phone application.

@carljm
Copy link
Collaborator Author

carljm commented Dec 17, 2020

Good suggestions, thanks! Implemented both, and also (per suggestion from @Zburatorul ) made the fees/capital-gains/dividend account metadata optional, falling back to FIXME_ACCOUNT, and added taxes_account metadata for consistency.

Still tox clean after these changes.

@jbms jbms merged commit cfd9b4e into jbms:master Dec 17, 2020
@jbms
Copy link
Owner

jbms commented Dec 17, 2020

Thanks!

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

4 participants