-
Notifications
You must be signed in to change notification settings - Fork 5
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
added support for processing schwab's tax statement #22
base: main
Are you sure you want to change the base?
Conversation
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed (or fixed any issues), please reply here with What to do if you already signed the CLAIndividual signers
Corporate signers
ℹ️ Googlers: Go here for more info. |
Hi Chetan, Thanks for the pull request to add support for Schwab! The reason that you're seeing the Google CLA on this PR is that even though this is my personal project, I was employed by Google when I created it (and still work there), so the Google CLA still applies. Please follow the instructions in the comment from the bot to address the CLA requirement, and then I'll be happy to review and merge your PR. Let me know if you have any questions or run into any issues with the CLA. Thanks again for your contribution! Looking forward to including it in the project. Best, |
Hey @243826, thanks again for your contribution! Can you please take a look at and sign the CLA? This is a required step before I can review your PR. If you have any questions or concerns, please let me know! |
googlebot I signed it! |
Thanks for signing the CLA, @243826! Since you're adding a converter for a new broker, would you be able to add one or two unit tests, perhaps using a simplified / anonymized version of the CSV file you have from Schwab, to make sure that we don't break the behavior of the converter with future changes, updates, or refactoring? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you very much for your contribution! Please see my comments below and let me know if you have any questions.
@@ -69,7 +69,10 @@ def __str__(self): | |||
|
|||
def txfDate(date): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe it's time we add type annotations to this code, it now takes str | datetime
and returns str
.
import utils | ||
|
||
|
||
FIRST_LINE = 'Description of property (Example 100 sh. XYZ Co.),Date acquired,Date sold or disposed,Proceeds,Cost or other basis,Accrued market discount,Wash sale loss disallowed,Short-Term gain loss Long-term gain or loss Ordinary,Form 8949 Code,Check if proceeds from collectibles QOF,Federal income tax withheld,Check if noncovered security,Reported to IRS: Gross proceeds Net proceeds,Check if loss is not allowed based on amount in 1d,Profit or (loss) realized in 2020 on closed contracts,Unrealized profit or (loss) on open contracts-12/31/2019,Unrealized profit or (loss) on open contracts-12/31/2020,Aggregate profit or (loss) on contracts,Check if basis reported to IRS,Bartering,State name,State identification no,State Tax Withheld\n' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For ease of readability, would you mind splitting these into a list of fields, and keeping them in a list, and joining them via ','.join(['Description of property ...', 'Date acquired', ... ]) + '\n'
?
return "Charles Schwab" | ||
|
||
@classmethod | ||
def washSaleDisallowedAmount(cls, dict): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We've stopped using dict
as a variable as it's also a type, please replace dict
with txn
and add a type annotation, it's most likely dict[str, str]
here and below. See other broker files for an example.
|
||
class Schwab: | ||
@classmethod | ||
def name(cls): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add a return type for this and other methods below.
* partial lot sales | ||
""" | ||
|
||
import csv |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add above:
from __future__ import annotations
This will make it easier for you to add type annotations below.
|
||
|
||
BROKERS = { | ||
'amtd': TDAmeritrade, | ||
'ib': InteractiveBrokers, | ||
'tdameritrade': TDAmeritrade, | ||
'vanguard': Vanguard, | ||
'schwab' : Schwab |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please terminate this line with a comma as above to make it easier to add / remove brokers in the future, without modifying other lines around them.
TRANSACTION_TYPE = 'Trans type' | ||
|
||
|
||
class Schwab: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As mentioned in my comment on the PR, could you please add a simple unit test for this file format?
See the files in testdata
directory for the format. make regen
updates expected test data outputs, see update_testdata.py
for what it actually does in that step.
No description provided.