-
Notifications
You must be signed in to change notification settings - Fork 304
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
prefer 'HashMap' to 'BTreeMap' when ordering isn't important #180
base: master
Are you sure you want to change the base?
Conversation
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 |
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. |
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. |
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 ofString
. Should provide some (very) modest performance gains.shame the benchmarks aren't working... *cough* #164 *cough*