-
Notifications
You must be signed in to change notification settings - Fork 1
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Feat/mailgun entry point #94
Open
Dovchik
wants to merge
10
commits into
milestone/mailgun-support
Choose a base branch
from
feat/mailgun-entry-point
base: milestone/mailgun-support
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
10 commits
Select commit
Hold shift + click to select a range
867d238
feat: add url resolver and entry point method in SinchClient.cs
Dovchik 04ad96a
chore: swap us and eu region in enum
Dovchik 91a98fa
chore: add TBD note
Dovchik 7c7c76b
add inhdoc to mailgun creator method
Dovchik 8a5a048
refactor: mailgun resolve test
Dovchik 0d7eb5a
test: check for unexpected enum value in region
Dovchik 370da9d
feat: provide parameterless constructor for SinchClient
Dovchik d5f1d38
chore: cleanup
Dovchik 7429aa7
feat: extend empty constructor with options
Dovchik 641a771
fix: doccomment
Dovchik File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,18 @@ | ||
namespace Sinch.Mailgun | ||
{ | ||
/// <summary> | ||
/// Mailgun allows the ability to send and receive email in both US and EU regions. | ||
/// Be sure to use the appropriate region on which you have created your domain. | ||
/// </summary> | ||
public enum MailgunRegion | ||
{ | ||
/// <summary> | ||
/// United States region | ||
/// </summary> | ||
Us, | ||
/// <summary> | ||
/// Europe region | ||
/// </summary> | ||
Eu | ||
} | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,23 @@ | ||
using System; | ||
using Sinch.Core; | ||
using Sinch.Logger; | ||
|
||
namespace Sinch.Mailgun | ||
{ | ||
/// <summary> | ||
/// The Mailgun API is part of the Sinch family and enables you to send, track, and receive email effortlessly. | ||
/// </summary> | ||
public interface ISinchMailgunClient | ||
{ | ||
// TBD: add domains | ||
} | ||
|
||
/// <inheritdoc /> | ||
internal class SinchMailgunClient : ISinchMailgunClient | ||
{ | ||
public SinchMailgunClient(Uri baseUrl, Http http, LoggerFactory? loggerFactory = null) | ||
{ | ||
// TBD: implement domains | ||
} | ||
} | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,28 @@ | ||
using System; | ||
using FluentAssertions; | ||
using Sinch.Mailgun; | ||
using Xunit; | ||
|
||
namespace Sinch.Tests.Mailgun | ||
{ | ||
public class MailgunClientTests | ||
{ | ||
[Fact] | ||
public void InitMailgunClient() | ||
{ | ||
var mailgun = new SinchClient().Mailgun("apikey", MailgunRegion.Eu); | ||
|
||
mailgun.Should().NotBeNull(); | ||
} | ||
|
||
[Theory] | ||
[InlineData("")] | ||
[InlineData(null)] | ||
public void FailEmptyApiKey(string apiKey) | ||
{ | ||
var mailgunCreation = () => new SinchClient().Mailgun(apiKey, MailgunRegion.Eu); | ||
|
||
mailgunCreation.Should().Throw<ArgumentNullException>(); | ||
} | ||
} | ||
} |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please, take a look here @asein-sinch regarding design of Sinch entry point
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FYI, if we agree on that, will move it to a separate PR to include it in version without mailgun
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As stated previously, It will require
SinchOptions
on the entry point of theSinchClient
non the less. That's whySinchClient().Mailgun(options?)
will not flawlesly here, as well as I'm lacking understanding what problem it will solve.Two options I'm fine with:
SinchClient(key?, secret?, projectId?, options?)
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 isSinchOptions
, 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 likeConversation
? There it is still a need forUrlOverride
. Should I rewrote all design toSpecificApi(Options)
? to have consistency, breaking backward compatibility. Or leaveSinchOptions.UrlOverride
as is, now having two places where you can setupUrlOverride
- in constructor of specific API AND in a globalSinchOptions
? 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 optionalSinchOptions.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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?