Feat/mailgun entry point

[Dovchik]

No description provided.

[asein-sinch]

There shouldn't be any logic in this test function: here you are recoding the logic from the UrlResolver. It would be better to provide the expected value as part of the test data. And also adding a test name to improve the readability, such as:

public static IEnumerable<object[]> ResolveMailgunUrlTestCases => new List<object[]>
{
    // Format: TestName, Region, Override, ExpectedUrl
    new object[] { "US Region Default URL", MailgunRegion.Us, null, "https://api.mailgun.net" },
    new object[] { "EU Region Default URL", MailgunRegion.Eu, null, "https://api.eu.mailgun.net" },
    new object[] { "EU Region with Null Override", MailgunRegion.Eu, new ApiUrlOverrides { MailgunUrl = null }, "https://api.eu.mailgun.net" },
    new object[] { "EU Region with Custom Override", MailgunRegion.Eu, new ApiUrlOverrides { MailgunUrl = "https://my-mailgun-proxy.net" }, "https://my-mailgun-proxy.net" },
};

The test would be more straightforward:

[Theory]
[MemberData(nameof(ResolveMailgunUrlTestCases))]
public void ResolveMailgunUrl(string testName, MailgunRegion region, ApiUrlOverrides apiUrlOverrides, string expectedUrl)
{
    var actual = new UrlResolver(apiUrlOverrides).ResolveMailgunUrl(region);
    actual.Should().BeEquivalentTo(new Uri(expectedUrl));
}

And in order to test all the branches from the UrlResolver method, you can consider adding another test for an unexpected Region value.

[Dovchik]

Agree, refactored the test

And in order to test all the branches from the UrlResolver method, you can consider adding another test for an unexpected Region value

for a note, MailgunRegion is a enum, it's impossible to add pass unexpected region value

[asein-sinch]

even with ResolveMailgunUrl((MailgunRegion)(-1)) ?
I thought this was why you added a default clause in the switch statement in the ResolveMailgunUrl method

[Dovchik]

It's possible with enum casting, yes, even if it's very unlikely, but totally possible, someone going to do that, I'll add that to the test case.
default is for any unexpectancy, yes

[asein-sinch]

I know it's following the same design as Verification and Voice which are not using the same kind of credentials, but it still seems very weird to have to initialize a SinchClient with a triplet of undefined parameters to be able to access to the Mailgun constructor with its own parameters; knowing that on top of that, the overrides for the URL should be part of the optional fourth parameter of the SinchClient.
Do you think it would be valuable to review this design?

[Dovchik]

Agree, this seems somewhat weird, I was thinking about adding an empty constructor new SInchClient() for such a use case, but dropped it for the sake of not providing confusion of two constructor. But I can add it and indicate with a doc comments that an empty constructor is for use of specific APIs in case you need only them. What do you think?
P.S. I don't want to go into a direction of static constructor so two calls of different apis, e.g. sinchClient.Sms.Send() and sinchClient.Mailgun("key").Send() would share the resources.

[asein-sinch]

I'm not sure to understand what you want to do, but let's go ahead and give it a try.

[Dovchik]

Please, take a look here @asein-sinch regarding design of Sinch entry point

[Dovchik]

FYI, if we agree on that, will move it to a separate PR to include it in version without mailgun

[asein-sinch]

It's interesting to have a SinchClient constructor with no required parameters but it becomes ever more confusing to let the SinchOptions at this level

[Dovchik]

Yeap, that's why I'm content with current design of having only one entry point, even with "weird" feel if you only need other API.
Nonetheless, I'm open to other suggestions, but can we, please, move this discussion as out of scope of this PR and discuss it elsewhere and for other task/PR?

[asein-sinch]

I think this is precisely the goal of this PR to address the Mailgun entry point as the name suggests. Would it make sense to define the options in the Mailgun() constructor: Mailgun(key, region, options?)

[Dovchik]

As stated previously, It will require SinchOptions on the entry point of the SinchClient non the less. That's why SinchClient().Mailgun(options?) will not flawlesly here, as well as I'm lacking understanding what problem it will solve.
Two options I'm fine with:

• As was before SinchClient(key?, secret?, projectId?, options?)
• The proposition with an empty SinchClient(options?) for speficic APIs, in separate PR as it affects whole SDK - to clarify, before merging Mailgun to main, this change will land here eventually.

Again, I'm open to other designs, if any.

[asein-sinch]

SinchClient(sinchOptions).Mailgun(key, region, url)
SinchClient(sinchOptions).Mailgun(key, region, mailgunOptions)

The idea is to not pollute the SinchClient with options for a non-standard API and to not have to manage the Mailgun configuration in various places

[Dovchik]

Idea makes sense, but bear with me:

• In case you have an optional SinchOptions.UrlOverrides where you can setup all urls you need in one place and other optionals is SinchOptions, where you will look for any similar stuff.

• In case of SinchClient(sinchOptions).Mailgun(key, region, mailgunOptions/url), and we will be talking about optional urlOverride specifically, you will be overriding near the entry point, and I like the idea. But now, what about non-specific api like Conversation? There it is still a need for UrlOverride. Should I rewrote all design to SpecificApi(Options)? to have consistency, breaking backward compatibility. Or leave SinchOptions.UrlOverride as is, now having two places where you can setup UrlOverride - in constructor of specific API AND in a global SinchOptions? A for me, having to group similar settings makes sense. . Also, Options pattern is widely used in dotnet ecosystem, and intuitively, devs gonna to look into it to see what's there.
  Also, you probably can ask: then why Mailgun(creds, region) takes place. And answer to that is I want to force here a conscious decision for a required properties. While optional SinchOptions.UrlOverrides is for a specific purpose.

In general, it comes down to personal preference.
The source of a problem is a variety of SInch products and we trying our best to provide a somewhat unified experience.
And to state again, this design applies not only to Mailgun, but affects all SDK.

[asein-sinch]

In general, it comes down to personal preference.

TBH a documented team decision would be preferable.
If the decision leads to a breaking change, then we can plan for a v2 with the delivery of the mailgun support in the SDK.

[Dovchik]

I didn't get what you mean here.
Should we include doc team to discussion?
Any more comments about design? Do you want to continue discussion?