intercom / intercom-dotnet

Intercom API client library for .NET
https://developers.intercom.io/reference
Apache License 2.0
63 stars 54 forks source link

Ic added rest client factory #145

Closed ianintercom closed 5 years ago

ianintercom commented 5 years ago

Why?

ianintercom commented 5 years ago

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 (https://github.com/intercom/intercom-dotnet/issues/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.