Skip to content
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

Strongly typed #211

Closed
wants to merge 58 commits into from
Closed

Strongly typed #211

wants to merge 58 commits into from

Conversation

Jericho
Copy link

@Jericho Jericho commented Mar 24, 2016

  • Values returned by methods are strongly typed
  • Sensible method names. For example, "Create" instead of "Post".
  • All methods are awaitable
  • Avoid deadlocks in ASP.NET by making sure all async calls is configured with ConfigureAwait(false)

…to be able to handle content with any HTTP verb (as opposed to only POST and PATCH).

These two changes are necessary in order to handle calls such as 'Delete Multiple lists' for example which allows an array of integers to be in the body of the DELETE request.
Also allow the HttpClient to be injected (this is useful for mocking and for using Fiddler)
Added overloaded Client.Delete methods to accept JObject in addition to JArray
Added EpochConverter to convert Unix time to .NET DateTime when parsing JSON
Added convenient extension methods to convert unix time to .NET DateTime
Refresh my repo with recent commits to SendGrid's repo
…to be able to handle content with any HTTP verb (as opposed to only POST and PATCH).

These two changes are necessary in order to handle calls such as 'Delete Multiple lists' for example which allows an array of integers to be in the body of the DELETE request.
Also allow the HttpClient to be injected (this is useful for mocking and for using Fiddler)
Added overloaded Client.Delete methods to accept JObject in addition to JArray
Added EpochConverter to convert Unix time to .NET DateTime when parsing JSON
Added convenient extension methods to convert unix time to .NET DateTime
Most of the time, 'new JArray(...)' is fine but there are a few exceptions (such as where serializing the Versions property of the Template class) therefore I decided to standardize on JArray.FromObject
@Jericho
Copy link
Author

Jericho commented Mar 24, 2016

I just fixed the unit tests but I just wanted to point out that they are not really unit tests since they make actual calls to the SendGrid API. These unit tests should be improved to test our C# code (with mocked httpclient) as opposed to making API calls.

@Jericho
Copy link
Author

Jericho commented Mar 25, 2016

Unit tests are failing because of the following error:
{"errors":[{"field":null,"message":"authorization required"}]}

I'm guessing this is due to the fact that "SENDGRID_APIKEY" environment variable is missing from the build machine.

@thinkingserious
Copy link
Contributor

You are correct. I thought I fixed that issue though.

In any case, I'm refactoring the unit tests in the new library so that they no longer require a network call. I'm thinking I'll have the integration tests only run locally. The new HTTP client is already refactored in that way with a mocked client: https://github.com/sendgrid/csharp-http-client/blob/master/UnitTest/UnitTest.cs

@thinkingserious thinkingserious added type: community enhancement feature request not on Twilio's roadmap status: cla needed labels May 18, 2016
@thinkingserious
Copy link
Contributor

Hi @Jericho,

We've finally gone to beta with our new library and you can check it out here: https://github.com/sendgrid/sendgrid-csharp/tree/v3beta

We plan to implement strong typing through helpers, see the helper for v3 /mail/send for an example: https://github.com/sendgrid/sendgrid-csharp/blob/v3beta/SendGrid/SendGrid/Helpers/Mail/Mail.cs Currently, we've just added the getters/setters in preparation for that.

If you would like to continue contributions, we will need your pull requests to be made against the v3beta branch and we'll need a signed CLA: https://github.com/sendgrid/sendgrid-csharp/blob/v3beta/CONTRIBUTING.md#cla Please let me know if you have any questions.

We appreciate your patience and continued support!

With Best Regards,

Elmer

@thinkingserious
Copy link
Contributor

Thanks for the CLA @Jericho!

Will you be making a new pull request on our v3beta branch or will wait until it gets out of beta in a few weeks?

@Jericho
Copy link
Author

Jericho commented May 20, 2016

I am trying to figure out if it’s possible to change the branch for a pull request. Maybe the only solution is to close the original PR and create a new one?? Do you have any suggestion?

@Jericho
Copy link
Author

Jericho commented May 20, 2016

Also there is a branch called v3_beta and another one called v3beta. Which one is the right one?

@thinkingserious
Copy link
Contributor

The correct branch is v3beta, thanks for checking!

That branch is a complete rewrite of this library, so I don't think this pull request can be directly mapped.

I'm happy to help figure it out with you, but that would require some time as I'm now in the process of finishing updating all our 7 libraries in support of the v3 /mail/send endpoint and coverage for all v3 Web API calls. That work comes out of beta in a few weeks, so my guess would that I would need at least a few weeks before I can dig in deeper with you. That said, if you come up with any specific questions, please let me know and I'll do my best to answer quickly

Thanks for checking in and for your support!

With Best Regards,

Elmer

@thinkingserious
Copy link
Contributor

I should also note that the only helper that has been implemented so far is for the /mail/send endpoint, which you can find here: https://github.com/sendgrid/sendgrid-csharp/tree/v3beta/SendGrid/SendGrid/Helpers/Mail. Strong typing has not been implemented there, but the framework is in place. You would be using a similar strategy for strongly typing any other endpoint.

Please let me know if you have any questions.

@thinkingserious
Copy link
Contributor

Now that we are out of beta, I suggest you go ahead make a new pull request against the current version.

Thanks @Jericho!

Please let me know if there is anything I can do to help.

gabrielkrell pushed a commit to gabrielkrell/sendgrid-csharp that referenced this pull request Aug 2, 2017
Refactored attachments(...) in sendgrid/helpers/inbound/parse.py, doc…
gabrielkrell pushed a commit to gabrielkrell/sendgrid-csharp that referenced this pull request Aug 2, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: community enhancement feature request not on Twilio's roadmap
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants