search

nash-io / openlimits

A Rust high performance cryptocurrency trading API with support for multiple exchanges and language wrappers.
http://openlimits.io
BSD 2-Clause "Simplified" License
275 stars 44 forks source link

Test and code guidelines improvements #129

Closed notdanilo closed 3 years ago

notdanilo commented 3 years ago

Here are the points to review in this PR:

  1. I created a test_subscription_callback function to get rid of code repetition in the subscription callback tests.
  2. I added few conventions to the contribution document. It might be worth discussing.
  3. I replaced all the unwrap in the code with expect. I found some usages of unwraps in the actual library code, they certainly assume the unwrap/expect will never fail.
  4. I also removed some println!s that were polluting the test command output.5. This is quite unrelated to the purpose of this PR, but I just renamed NashStream to NashWebsocket to match the naming to BinanceWebsocket and CoinbaseWebsocket. But maybe we want to do it in the opposite direction?

Regarding 3.: There are still some println! in the tests that should be removed and replaced with proper tests. But it seemed a lot of work to do in this PR. I will leave it for later and go back to #121

jankjr commented 3 years ago

LGTM.

I think we can merge it as is as it only improves on exceptional cases. Any objections @Ejhfast ?

Ejhfast commented 3 years ago

Awesome work, looks good to me as well.