search

wisespace-io / binance-rs

Rust Library for the Binance API
Other
636 stars 287 forks source link

prefer 'HashMap' to 'BTreeMap' when ordering isn't important #180

Open danieleades opened 1 year ago

danieleades commented 1 year ago

not sure why BTreeMap has been used instead of HashMap here, given that there's no need to preserve ordering (is there?).

I've also removed some allocations by using &'static str for the keys instead of String. Should provide some (very) modest performance gains.

shame the benchmarks aren't working... *cough* #164 *cough*

danieleades commented 1 year ago

i'd actually like to go even further, and use concrete structs to define the parameters, rather than a stringly-typed map.

i've got a Monzo API client that shows the approach i'm talking about - https://github.com/danieleades/monzo-lib/blob/609ff145995ab68e798c25ec43d27f6e4648a366/src/endpoints/transactions/list.rs#L93

I'll leave that for another PR

danieleades commented 1 year ago

this change causes a lot of tests to fail. I'm guessing because the way mockito is used, the order of query parameters is significant. I'll have to have a think about the best way to resolve this. In my opinion, this is an issue with the testing, not with the implementation.

danieleades commented 1 year ago

i've fixed a few of the tests, but it's very time consuming. I'll mark this is a draft until I have time to look at this properly. As expected, the key is refactor the tests such that query parameter ordering is not significant (by using mockito::Matcher::AllOf).