Ic added rest client factory

[ianintercom]

Why?

• The existing architecture that we had made it very hard to test what was going on with the clients
• With each client in charge of constructing their own RestSharp clients, there was no way to mock server responses to test the parsing logic.
• I think this kind of breaking change would be a major version bump

How?

• Introduced the concept of a factory for RestSharp client instances.
• Made all clients take this as a single dependency.
• Used this to dependency to mock the RestSharp clients comms with the server.
• Added single POC test for viewing a user illustrating the usage of the mock and our new ability to test the output of the client.
• Bumped version to 3.0.0

[ianintercom]

This test is key to demonstrate the power of the new pattern

[jmfryan]

Nice, this was something I noticed when browsing the codebase. Making things more testable is certainly a good idea.

I have a few of questions for discussion:

1. Do we really need the factory. Could we inject a Rest client directly? Seems like the factory is just a container for BASE_URL

2. Are we able to make this change in a backward compatible way? Overloaded constructors and defaulting to constructing our own client may be possible and would allow us to avoid the major version change.

3. If we cant avoid the major version change, are there any other major changes we want to make? We dont want to break all our clients and then break them again in a few weeks

[ianintercom]

Hey Jack, thanks for the feedback, this is great! To address your points:

1. We don't strictly need the factory but, it gives us a few things
   • It allows us to inject our http client but allows allows us to do it in a way decoupled from RestSharp. So our consumers don't need to know anything about RestSharp it's self.
   • It also allows us to control the headers etc and the config of the RestClient. If it were injected by the consumer it would be easier to miss-configure it.
   • There is also an open issue about making the client class disposable (Please make client class disposable and call httpClient.Dispose() #134), if we decide to go ahead with this, it would be good to have control over the creating of the HttpClient via the RestClient instance
2. We probably can and add a deprecation warning to the old style constructor. This is probably the best way to do this really 👍
3. For me, I think this is one of the biggest and most important changes to be made in terms of core architecture. There is an outstanding task to support async and await but this won't be a breaking change. While it would be major work, it wouldn't be breaking anything.

[choran]

👍