Feat/support service plan #47
Conversation
Dovchik
requested review from
650elx,
krogers0607,
asein-sinch and
JPPortier
as code owners
| @@ -0,0 +1,33 @@ | |||
| namespace Sinch.SMS | |||
There was a problem hiding this comment.
Not sure this will be an added value from SDK user's perspective : defining a dedicated type for supported region in case of service plan id usage
This mean if for test or any other reason user switch from projectId <-> servicePlanId usage the region as also to be changed. Not because wanting to changed to the region but because type is different.
e.g.: Still addressing us region in both cases but have to change SmsHostingRegion.Us <-> SmsServicePlanIdHostingRegion.Us
Even if supported regions are not equals, a dynamic validation against unexpected values should be enough.
And even in this case: it should be a warning and not an error. So in case of server going forward to support new region, SDK's user do not need to move to a new (but not yet ready release) SDK updated version
There was a problem hiding this comment.
I'll keep dedicated type for regions as sets of supported are different. For any 'I don't want checks' users it's a record so new SmsServicePlainIdHostingRegion('cn') should work.
There was a problem hiding this comment.
From the SDK user's point of view, a region is just a region: the EU, the USA, ... are no different targets when approached by service plan or project.
What is the added value of complexity for SDK's user?
There was a problem hiding this comment.
The supported set of regions are different, and different enums reflects just that.
There was a problem hiding this comment.
This has to be discussed within the team. Item added in the parking lot
src/Sinch/SinchClient.cs
Outdated
| httpSnakeCase); | ||
| } | ||
|
|
||
| private static Uri GetSmsBaseAddress(SmsServicePlanIdHostingRegion smsServicePlanIdHostingRegion, |
There was a problem hiding this comment.
Related to previous comment about different types for same region usage: method signature based onto region type is used to trigger servicePlanId vs projectId URL.
This should be related to servicePlanId usage not onto region type usage
There was a problem hiding this comment.
I'm renaming the method to better reflect underlying functionality
Add support for Service plan id of sms api. Which can be set via SinchOptions