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: orderbook calculation bugs, expose more data #442

Merged
merged 9 commits into from
Jun 24, 2024

Conversation

tyleroooo
Copy link
Contributor

No description provided.

Copy link
Contributor

@yogurtandjam yogurtandjam left a comment

Choose a reason for hiding this comment

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

looks good but would really appreciate some unit tests to illustrate any edge or confusing cases we're concerned with. i'm not as familiar with this part of the code and there's a lot of logic here so i'm hesitant to stamp

@@ -510,43 +517,55 @@ internal class OrderbookProcessor(parser: ParserProtocol) : BaseProcessor(parser
}

fun group(orderbook: List<Any>?, grouping: Double): List<Any>? {
return if (orderbook != null && orderbook.size > 0) {
return if (!orderbook.isNullOrEmpty()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

[discussion/suggestion] IMO this is one those functions that has enough math and logic where it makes sense to test separately. we should probably be actively testing edge cases and difficult to reason-about situations like stuff that's been commented here.

i tried to start a pattern with adding the processor tests to the commonTest folder.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think the existing orderbook tests hit the main points pretty well............and I feel like it would take me forever to set up new tests.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah the orderbook tests basically hit this directly

@tyleroooo tyleroooo merged commit 0a728aa into main Jun 24, 2024
4 checks passed
@tyleroooo tyleroooo deleted the tu/fix-orderbook-calc branch June 24, 2024 20:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants