Skip to content

Conversation

nagy
Copy link
Contributor

@nagy nagy commented Jun 19, 2025

On (knockout-)options, before this it would assign the ISIN of the underlying instrument instead of the ISIN of the option itself and the amount of options was also missing.

If you want I can also craft a testcase for this.

@nagy nagy marked this pull request as ready for review June 19, 2025 09:23
@RealCLanger
Copy link
Collaborator

I guess this makes sense overall.

It would be good to have a test case with an added example event, also to have some data at hand for developers that don't have such events at hand in their own account.

@nagy nagy force-pushed the dividends-knockouts branch from 7ce0959 to bcfa8aa Compare June 21, 2025 18:01
@nagy
Copy link
Contributor Author

nagy commented Jun 21, 2025

I have added a test for this.

Copy link
Collaborator

@RealCLanger RealCLanger left a comment

Choose a reason for hiding this comment

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

Thanks for adding the test. I went through the code and have some suggestions.

@nagy nagy force-pushed the dividends-knockouts branch from bcfa8aa to 04ebc5f Compare July 2, 2025 09:01
@nagy
Copy link
Contributor Author

nagy commented Jul 2, 2025

The tests seem to pass and it is a lot cleaner now. Thank you.

@RealCLanger
Copy link
Collaborator

I just submitted larger changes for parsing to the mainline. Can you merge these changes into your PR, then we can finalize this.

On (knockout-)options, before this it would assign the ISIN of the
underlying instrument instead of the ISIN of the option itself and the
amount of options was also missing.
@nagy nagy force-pushed the dividends-knockouts branch from 04ebc5f to 4fabea4 Compare July 20, 2025 12:48
@nagy
Copy link
Contributor Author

nagy commented Jul 20, 2025

Done. Thank you for your work.

@RealCLanger RealCLanger merged commit 29391a7 into pytr-org:master Jul 21, 2025
5 checks passed
@RealCLanger
Copy link
Collaborator

Done. Thank you for your work.

Thank you, I've merged it.

@nagy nagy deleted the dividends-knockouts branch July 21, 2025 16:16
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.

2 participants