Dynamic Properties

[mskutta]

Thanks for putting together this API. I have a quick suggestion...

Is there a reason behind using dynamic properties? I feel like it makes the API difficult to work with. It is not easy to discover how to use the API without referring to documentation or debugging.

Thanks!
Mike

[thinkingserious]

Hello @mskutta,

Thank you for the question!

That portion of the API client is meant to make it easy to call any v2 Web API endpoint we have now or will release.

To solve the issue you are having, we are using the concept of Helpers. You can see the first one we have developed here: https://github.com/sendgrid/sendgrid-csharp/tree/master/SendGrid/SendGrid/Helpers/Mail. It is a first iteration and we welcome suggestions on improvement and also ideas on which endpoints you would like to see have helpers also. Pull requests are also appreciated :)

Further information about the design is here: https://sendgrid.com/blog/using-c-to-implement-a-fluent-interface-to-any-rest-api

Please let me know if you have further questions.

With Best Regards,

Elmer

[ohadschn]

To my mind this is an abuse of the dynamic keyword. It was added to .NET specifically in order to improve interoperability with dynamic languages and frameworks. If you want to call some python code dynamic is cool, but a "dynamic fluent" API is simply not worth the loss of type safety, and intellisense along with it. Discoverability is also shot, as the code is now impossible to decipher with ILSpy (which is much faster to navigate than GitHub).

As for the helper library, now that a Mail class is available, I suggest some type-safe wrapper for sending the e-mail, preferably returning something like Task<HttpResponseMessage> (so we can await it without worries).

Thanks and sorry for the rant :)

[thinkingserious]

@ohadschn we love rants! Seriously, we do. It indicates you are a passionate developer who cares about your craft. We take such feedback very seriously.

As you can see, we are in the very early stages with this new library. We had to take a few steps back to support the v3 Web APIs and the plan is to iterate based on feedback from the .NET C# community. So far, we have received a nice amount of great feedback (including yours :)). You can further contribute by creating, upvoting or commenting on issues. These contributions help bump things up in our priority queue.

Within a few iterations, we hope the helpers will provide easy solutions for all the main use cases for the v3 Web API, effectively hiding the lower level implementation. The Mail helper was the first example, but it's only the first in many iterations. Your feedback will help guide its completion.

I like the idea about returning Task<HttpResponseMessage>. Do you want to take a crack at a pull request?

Thanks again for taking the time to let us know your thoughts!

[ohadschn]

@thinkingserious I love your positive attitude :)

I think the first order of business would be to merge this: sendgrid/csharp-http-client@391b060. It looks good to me. I realize this will be a breaking change, but I believe it's worth it, and better sooner than later right?

I then suggest we get rid of the SendGrid.CSharp.HTTP.Client.Response class - why not just return the HttpResponseMessage directly?

Finally, enabling the basic scenario of sending an email message in a type safe wrapper would become easy by adding a method like this to the Mail class:

Task<HttpResponseMessage> Send(string apiKey)
{
    dynamic sg = new SendGrid.SendGridAPIClient(apiKey);
    return sg.client.mail.send.post(Get());
}

And now all the client has to do is:
HttpResponseMessage response = await new Mail(from, subject, to, content).Send(apiKey);

So now all the dynamic stuff has been hidden away, and the client has a type-safe way to send emails 💯

[thinkingserious]

@ohadschn,

Once again, thanks for the detailed and actionable feedback. We would love to throw some swag your way! Could you please email us ([email protected]) with your T-shirt size and mailing address?

Now, on to the feedback!

First of all, thanks for the kind words! This stuff is great fun to me and I'm very pleased to be finally getting such detailed feedback. We don't want to create this library in a vacuum.

Ticket #9 is pretty high on our backlog right now, so I think it will be fixed in short order. Thanks again for giving another set of eyes on the code! With the breaking change, we'll just bump the major version number. I'll try to add in any other breaking changes too at that point.

I don't want to return HttpResponseMessage directly because we want to allow people to pass in their own HTTP client's but keep a common interface. Perhaps we can pass back an additional variable that includes the raw response from whatever client it being used. Thoughts?

Love the changes to the helper, this is exactly the direction we want to take. We do have a task for this in our backlog, but that one is a bit further down the list; however, I just added your voice to it for a bit of a priority bump. That said, the best way to get this one bumped in the near future is with a pull request.

[ohadschn]

@thinkingserious Hi again :)

Regarding the ticket, I assume you mean this one (on csharp-http-client). I'm happy to hear that, as using await is pretty cool!

Regarding the HttpResponseMessage, I think the biggest question is why? Basically everybody uses HttpClient. Besides, in this case the client is internal and it seems like System.Net.Http.HttpClient is created internally anyway. Furthermore, it is assumed that HttpResponseMessage is returned from the client. Don't get me wrong, I think that totally makes sense - anything else would force an implicit contract since there's no IHttpClient (e.g. rely on a given class to implement a method named SendAsync which could fail at runtime - not a good idea). Beyond that, I think a client typically won't care which HTTP client you use as long as it gets the job done - and HttpClient is quite battle tested.

Worst case scenario, the rare user that wants to use their own HTTP client can just fall back to the REST API, it's really not so hard to do something like: myClient.Post("https://api.sendgrid.com/v3/mail/send", "Authorization: Bearer $API_KEY"", mail.Get());. In fact, that's exactly what I'm going to do until the async/await fix is in. I can even do it as an extension method on Mail so I get the exact same call I ended up in my previous post...

Consequentially, it's not that urgent for me to work on that pull request, and I do have other projects to attend to (including my RL full time job :)), but once the async/await fix is in I'll definitely consider it.

[thinkingserious]

@ohadschn,

Thanks for the thoughtful and compelling feedback.

Please do follow this ticket for the upcoming fix: sendgrid/csharp-http-client#9

[ohadschn]

Will do, and thanks for listening :)

[BrianVallelunga]

I just came here looking for help with the new API. I think if you're going to produce a .NET API client, it should be strongly typed for the majority of common functionality. Having everything be dynamic is neither discoverable nor testable. The helpers are a good idea, but I would really consider making the helpers to be the main API and having the dynamic objects as the internal core that is rarely used.

[thinkingserious]

@BrianVallelunga,

You've got it right, that's the plan and this is just the beginning of many iterations. We had to take a few steps back to get the API library decoupled from the first mail/send endpoint and provide access to all v3 endpoints.

The best way you can help today is by up-voting and/or commenting on issues. Pull requests are also welcome :)

[saluce65]

I'm actually currently working on creating objects to represent the data being sent to SendGrid, which will be compatible with ASP.NET Core and Full Frameworks. Once I've completed that project, I'll generate a pull request.

[ohadschn]

A couple of relevant comments:

1. The SendGrid HTTP client is not really necessary, we can just do something like this:

var client = new HttpClient(); //this can be a static/singleton somewhere
client.DefaultRequestHeaders.Authorization = new AuthenticationHeaderValue("Bearer", ApiKey);
client.BaseAddress = new Uri("https://api.sendgrid.com/v3/");
//one time setup is done, we can now call from anywhere like:
var mail = new Mail(from, subject, to, content);
await client.PostAsync(
    "mail/send", new StringContent(mail.Get(), Encoding.ASCII, "application/json"));

2. Using HttpClient you actually have a way of letting your users use their own clients - all they have to do is implement HttpMessageHandler using the client of their choice.

[thinkingserious]

@ohadschn,

Interesting.

Since the SendGrid.CSharp.HTTP.Client is just a thin wrapper around HttpClient, it should not be difficult to do item 2 and it might be time to just combine the two repos.