-
Notifications
You must be signed in to change notification settings - Fork 43
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 parsing support for gift transactions (Schwab Equity Awards JSON). #539
base: main
Are you sure you want to change the base?
Conversation
@@ -199,6 +202,7 @@ def __init__(self, row: JsonRowType, file: str, field_names: FieldNames) -> None | |||
quantity = _decimal_from_number_or_str(row, names.quantity) | |||
amount = _decimal_from_number_or_str(row, names.amount) | |||
fees = _decimal_from_number_or_str(row, names.fees) | |||
currency = "USD" |
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.
Why have you moved it?
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.
I wanted to set currency
to None
, but then realised that it's a (non-optional) str
in BrokerTransaction
(https://github.com/KapJI/capital-gains-calculator/blob/main/cgt_calc/model.py#L83), so I reverted that change and forgot to undo this change to where the default is specified. I'm happy to undo this move.
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.
Yeah, let's undo.
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.
done, or well, undone.
@@ -305,6 +305,10 @@ def convert_to_hmrc_transactions( | |||
self.add_disposal(transaction) | |||
if self.date_in_tax_year(transaction.date): | |||
total_sells += self.converter.to_gbp_for(amount, transaction) | |||
elif transaction.action is ActionType.GIFT: | |||
raise NotImplementedError( |
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.
What's the point of adding it if it errors here?
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.
I thought NotImplementedError
was a more appropriate error message than the InvalidTransactionError
that IIRC it otherwise produces (as the transaction itself is valid), so it would be easier for users to understand why the tool isn't producing calculations.
If adding proper handling for gifts is out of the table, I think a better fix would be to raise an unknown transaction error in the parser if clarity is your concern. I see no need to add parsing for an unsupported transaction. |
This adds a new action type, as TRANSFER seems to deal with money transfers rather than share transfers, and SALEs also involve money whereas gifted shares do not. The calculation code will raise a NotImplementedError if it encounters one of these transactions. This may help KapJI#510. This contribution has been developed in my spare time.
It's not off the table - I plan to look into it but I don't really want to promise that I'll end up implementing it either at this stage.
I thought this would help progress support for gifts by one step, but also I don't mind if you want to hold off merging this until there's code using it for more than just error handling. |
This adds a new action type, as TRANSFER seems to deal with money transfers rather than share transfers, and SALEs also involve money whereas gifted shares do not.
The calculation code will raise a NotImplementedError if it encounters one of these transactions.
This may help #510. This contribution has been developed in my spare time.