Skip to content

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