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

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

Draft
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

danieleades
Copy link
Contributor

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
Copy link
Contributor Author

danieleades commented Oct 26, 2022

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
Copy link
Contributor Author

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
Copy link
Contributor Author

danieleades commented Oct 26, 2022

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).

@danieleades danieleades marked this pull request as draft October 26, 2022 23:53
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.

1 participant