Feature request: Don't use dynamic

[onionhammer]

Dynamic defeats the purpose and removes any advantages of a statically typed language. There is no compile time error checking, and there is no way to explore the API with autocomplete, you just have to know sg.client.mail.send.post(requestBody), which is kind of crazy.

Also it relies on the DLR, which is much slower than normal compiled/jitted C# code.

[thinkingserious]

Thanks @onionhammer!

Currently, our goal is to hide the implementation details through helpers. Here is our first try, but we have already received a ton of awesome feedback on how to improve: https://github.com/sendgrid/sendgrid-csharp/tree/master/SendGrid/SendGrid/Helpers/Mail).

Also, we want you to be able to pass in your desired HTTP client, this is why we removed it from the core of this library.

All that said, we've heard very compelling arguments ( thanks for adding another :) ) why we should ditch the reflection. So we will leave this ticket open and add it to our backlog. It can rise with additional +1s, comments or a pull request with a signed CLA.

[ryanelian]

I agree with @onionhammer. It wasn't immediately clear that the email sender method is asynchronous due to the lack of Intellisense.

In addition, the library should be written using C# Coding Convention. (e.g. instead of .client.mail.send.post, should be .Client.Mail.Send.Post instead!)

Also, we want you to be able to pass in your desired HTTP client, this is why we removed it from the core of this library.

I rather have the library statically typed than sacrificing static typing for plug-ability.

But let's say we want to implement that feature, we can design it like this instead:

public interface IHttpClientMapper
{
    Task PostAsync(object parameter1, object parameter2, object parameter3);
}

// Allow developer to implement their own mapper using the interface defined above,
// then set a static property containing a HTTP mapper for use by all SendGridApiClient instances!
SendGridApiClient.HttpClientMapper = new MyHttpClientMapper()

This way, the library is extensible without sacrificing static typing.

add it to our backlog

IMHO this should be a high priority issue, not a backlog. But hey, I don't run the company. 😄

[thinkingserious]

Hello @ryanelian,

Thanks for the feedback! I've added your vote to the issue.

With regards to the Async issue, we have: #200 (added your vote to that)

Intellisense should work once we get that integrated into the Mail Helper.

Regarding method naming conventions, we have: #239 (added your vote to that)

I believe we are using the proper naming conventions in the helper which will eventually hide the underlying HTTP client.

I like your suggestion for the IHttpClientMapper. We will revisit when it comes time to execute these changes.

It may be that we ditch the dependency to our dynamic client in favor of a static one.

Regarding the "backlog", I'm using that as a generic term to signify that it's something we will definitely execute as soon as we can. The priority is determined by a multitude of factors beyond what I described previously. I can say the priority on this is steadily rising and I'm looking forward to working on it again. We have a great deal of awesome feedback on this subject.

Thanks again!

[Corillian]

Please add my vote, I couldn't agree more that using dynamic makes the API clunky and unnecessarily difficult to use. There are so many levels of indirection within the client just to send an email and without intellisense you're forced to look at the source code to see what's going on. That's how I ended up here in the first place because I have no idea what the hell a response.StatusCode is, your documentation doesn't tell me, and now I have to dig through your code just to verify my suspicion that it's an integer status code from the web request since it could also be the .NET framework's strongly typed enumeration for HTTP status codes or something completely different. Pretty ridiculous.

Static typing is your friend and interfaces exist to hide implementations. I honestly can't think of a single good reason to use dynamic other than for consuming JSON objects, which is still dubious if the format of the JSON object never changes, as well as how ASP.NET uses it for ViewBag.

[saliehendricks]

+1

[Eirenarch]

I hope this release is some bad joke. Dynamic all the way? Conventions that go against the C# conventions (camelCase methods and properties)? It looks like someone let JS devs work on a C# library with no prior C# experience. Or maybe the library is autogenerated from swagger or something?

[thinkingserious]

Thanks everyone for all the +1's!

This is now scheduled to be the next C# issue we'll tackle when I return on Monday, followed by enabling support for .NET Core (during that process we will look into providing binaries for 4.5.X and 4.6.X.)

Before we begin writing code for the rewrite, I will collect all the feedback from here and other threads and start a new thread (I'll ping all of you here on that thread) where I'll outline the new approach. If you have the time, your feedback will again be appreciated at that point.

We REALLY appreciate your patience and support on this. The constructive criticism we have received on this has been awesome.

Please note that our scheduling could change, depending on what new issues arise until then. In any case, this has become near the top of our backlog, so it will get done soon.

@Corillian,

In the new version, we will move to using the framework's enumeration. The dependency on the dynamic HTTP client will be removed.

@Eirenarch,

Yes, the majority of this library is auto-generated from our Swagger definition. It was a shortcut to allow the support for the v3 Web API endpoints across all our libraries as a MVP in a short timeframe. The plan was to listen, then iterate accordingly. We will continue to listen and iterate, so please keep the feedback coming :)

[Eirenarch]

Honestly giving us a swagger generated library is kind of insulting. We know how to call rest services. Either provide statically typed API that prevents errors and provides good developer experience by virtue of having proper types and methods or tell us to call your REST services directly.

[onionhammer]

@Eirenarch I don't think this a very productive direction to take this issue discussion.

@thinkingserious Thanks for the good news - looking forward to your next updates

[thinkingserious]

@Eirenarch,

That is the idea behind the helpers. What you are asking for is the same thing we wish to deliver, we just need some time to get there. Thanks again for helping validate and pushing us forward.

Also, I forgot to mention that we built the original code and automation tools ourselves so that once we do get it right we can scale across all our libraries. We did look at using existing Swagger auto-generating tools, but decided that in the end that would not serve our end goals.

[thinkingserious]

Thanks for the kind words @onionhammer. I'm really excited about the next steps!

[saliehendricks]

Great to hear this is underway. Also very glad to hear about 4.5.0 libs
being supported. I just forked and compiled for 4.5 today and was annoyed
that I needed to do that for no apparent reason.

On 18 August 2016 at 22:05, Elmer Thomas notifications@github.com wrote:

Thanks for the kind words @onionhammer https://github.com/onionhammer.
I'm really excited about the next steps!

—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
#303 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/ABvuwmKSJCNhHPdBIO9KuuBZMIvXhUbfks5qhLsHgaJpZM4JjMem
.

[thinkingserious]

@saliehendricks ouch :(

We've heard that pain far too often, I can't wait to get that fixed too!

That said, we are planning to only officially support 4.5.2+ because those are the versions that Microsoft still supports.

May I ask why you are using 4.5.0? I'm asking to learn if we should indeed provide those dlls also. Thanks!