Skip to content

Conversation

@quantx-heiko
Copy link
Contributor

@quantx-heiko quantx-heiko commented Aug 1, 2025

I fixed the cash settlement issue and introduced a helper class with a method for checking if a symbol is cash settled.
Additionally I created unit tests for this method.
This PR relates to #46 .

@quantx-heiko
Copy link
Contributor Author

@laroche Did you have a look on this already. Is it OK for you?

laroche added a commit that referenced this pull request Aug 13, 2025
Fix some more cash settled options. This patch is
based on #49
from quantx-heiko.

Signed-off-by: Florian La Roche <[email protected]>
@laroche
Copy link
Owner

laroche commented Aug 13, 2025

Great, I've merged your code in, but didn't want to add a new file for this.
I haven't looked at the example lines yet, but hope to find some more time
the next days to look at example input for this.

Thanks a lot,

Florian La Roche

@quantx-heiko
Copy link
Contributor Author

Thanks, I introduced the helper class because I also implemented Unit Tests for the new method which is checking if the symbol is being cash settled or not.
I'd appreciate to see the unit tests as part of the master code as well. I makes it much more easier for all contributors to see if things break during developing new features or fixing bugs.

@laroche
Copy link
Owner

laroche commented Aug 17, 2025

Yes, unittests are missing completely right now.
One possibility would be to have a broad set of example trades together with verified/correct tax output
and make sure this set stays correct with all further changes coming in.

best regards,

Florian La Roche

@quantx-heiko
Copy link
Contributor Author

I absolutely agree, I already started implementing some unit tests with my first contributions to the project.
So I suggest to start with the Helper class and the unit test for cash settlement, I supplied with this pull request.

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