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

Fix Schwab parsing #459

Merged
merged 3 commits into from
Jan 29, 2024
Merged

Fix Schwab parsing #459

merged 3 commits into from
Jan 29, 2024

Conversation

amaramrahul
Copy link
Contributor

@amaramrahul amaramrahul commented Jan 5, 2024

Updated parser logic for Schwab

Fixes #451
Fixes #457
Fixes #464

@vmartinv
Copy link
Collaborator

Can you try again? I downloaded my transactions and it looks like Quantity and Price columns are swapped.

@amaramrahul
Copy link
Contributor Author

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.

@vmartinv
Copy link
Collaborator

vmartinv commented Jan 19, 2024

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:

    for upper_row, lower_row in zip(lines[::2], lines[1::2]):
        # in this format each row is split into two rows, so we combine them safely below
        row = []
        for upper_col, lower_col in zip(upper_row, lower_row):
            assert upper_col == "" or lower_col == ""
            row.append(upper_col + lower_col)

        if len(row) != len(header):
            raise UnexpectedColumnCountError(
                row, len(header), schwab_award_transactions_file or ""
            )

        row = dict(zip(header, row))
        date_str = row["Date"]

@amaramrahul amaramrahul force-pushed the schwab-fixes branch 7 times, most recently from 4ab38be to 74c02a8 Compare January 22, 2024 13:54
@amaramrahul
Copy link
Contributor Author

@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.

@vmartinv
Copy link
Collaborator

vmartinv commented Jan 22, 2024

I think it looks good but can you also add a validation to make sure the columns are as expected? Something like this:

    expected_header = {
        "Date",
        "Action",
        "Symbol",
        "Description",
        "Quantity",
        "FeesAndCommissions",
        "DisbursementElection",
        "Amount",
        "AwardDate",
        "AwardId",
        "FairMarketValuePrice",
        "SalePrice",
        "SharesSoldWithheldForTaxes",
        "NetSharesDeposited",
        "Taxes",
    }
    header = lines[0]
    if set(header) != expected_header:
        raise ParsingError(
            schwab_award_transactions_file,
            f"Missing or unknown columns in awards file: {expected_header.symmetric_difference(header)}",
        )

Otherwise a format change might go unnoticed.

@amaramrahul
Copy link
Contributor Author

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.

@amaramrahul amaramrahul force-pushed the schwab-fixes branch 6 times, most recently from 6fb4a99 to c413be6 Compare January 23, 2024 11:53
@amaramrahul amaramrahul changed the title Fixes #451 and #457 Fixes #451 #457 #464 Jan 23, 2024
@vmartinv
Copy link
Collaborator

vmartinv commented Jan 26, 2024

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 ?

@KapJI KapJI changed the title Fixes #451 #457 #464 Fix Schwab parsing Jan 29, 2024
@KapJI KapJI added the bug Something isn't working label Jan 29, 2024
@KapJI
Copy link
Owner

KapJI commented Jan 29, 2024

Looks good, thank you!

And thanks @vmartinv for help with reviewing this.

@KapJI KapJI merged commit d1864b5 into KapJI:main Jan 29, 2024
3 checks passed
@amaramrahul amaramrahul deleted the schwab-fixes branch February 1, 2024 03:18
@amaramrahul amaramrahul restored the schwab-fixes branch February 1, 2024 03:18
@amaramrahul amaramrahul deleted the schwab-fixes branch February 1, 2024 03:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
3 participants