EasyPost / easypost-csharp

EasyPost Shipping API Client Library for .NET and .NET Framework applications
https://easypost.com/docs/api
MIT License
66 stars 74 forks source link

Add back ability to instantiate model classes #380

Closed jmalatia closed 1 year ago

jmalatia commented 1 year ago

Feature Request Is New

Description of the feature

C# is typed language! Why would this library move backwards and now require the use of Dictionary<string, object>?

For those of us who have been using EasyPost for years (we've been using it for 8+ years)... it was a chore to get the EasyPost-csharp Official to a point it was "usable" for .net.core. Many of us had our own forks and/or used the Official but had helpers in our applications. We have been using v2.5.1.2 for years because it just works.

Lately we have been seeing this library getting some much needed attention! We were excited! Async has been added and finally C# naming guidelines are being used... so we thought it would be a good time to update. To our surprise we can no longer create and initialize the key models (address, shipment, parcel, etc.) but we have to use Dictionary<string, object>!

Why would a C# programmer want to do:

Address address = new Dictionary<string, object>()
{
    { "name", "Dr. Steve Brule" },
    { "street1", "179 N Harbor Dr" },
    { "city", "Redondo Beach" },
    { "state", "CA" },
    { "zip", "90277" },
    { "country", "US" },
    { "phone", "8573875756" },
    { "email", "dr_steve_brule@gmail.com" }
}

When you can do this:

Address address = new Address()
{
    Name =  "Dr. Steve Brule",
    Street1 =  "179 N Harbor Dr",
    City =  "Redondo Beach",
    State =  "CA",
    Zip =  "90277",
    Country =  "US",
    Phone =  "8573875756",
    Email =  "dr_steve_brule@gmail.com",
}

The later is type checked, can easily be passed around as a typed object and one does not have to worry about mis-spelling any of the key names { "nammee", "" },. Who wants unchecked code these days when your using a typed language.

Additionally, the latter is the C# way to do it and this is a C# sharp library.

I willing to bet that very few programmers implemented EasyPost C# using the Dictionary<string, object> pattern in their calls, outside of when they were first testing things out (because that was the first sample code on the readme page). See nopCommerce compare commit updating from v2 to v4 which shows use of typed objects.

Without public constructors for the Models you have made a lot more work for us in addition to untyping our projects. Simple example. A quick any dirty form to manually create an EasyPost Address.

<!-- basic html form -->
<form>
    <input type="text" id="name" name="name" />
    <input type="text" id="street1" name="street1" />
    <input type="text" id="street2" name="street2" />
    <!--- ..more fields... -->
</form>

which posts to this asp.net core controller method:

// V2 like sample
public async Task<IActionResult> Address(EasyPost.Models.Address address)
{
    // ---config code here ---
    address.Create();

    //address is now populated with id and other info from Easypost 
}

With V4 this is no longer possible because Address can not be instantiate, nor can the object Address be passed as a parameter to any EasyPost method.

To further make the point:

With regards to this, I don't understand why it was removed instead of just being renamed or just made an instance method? But this is besides the point as you have now created services, which is fine - but you should be able to create empty object models and pass in typed models no matter the code structure.

So please add back public default constructors for the Models and add overloads for the methods that also accept the Typed Models and then under the hood of this library you can manipulate the typed models back into to whatever type it needs (Dictionary<string, object>, Json, etc.).

nwithan8 commented 1 year ago

Hello, thank you for the feedback.

There was a lot of discussion we had regarding the decision to go to a parameter dictionary rather than using explicit parameters or constructable objects. Ultimately, we decided we wanted to keep this library similar to our other client libraries, which accept dictionary parameters. By doing so, it means we don't have to add explicit support for if/when the parameter structure changes for an API call, making this library more self-maintainable.

We also wanted to remove the ability to construct local objects (e.g. Address, Shipment), to avoid a potentially confusing local-vs-server-side data mismatch (i.e. using a locally created object that does not exist server-side). As a result, objects like Address and Shipment all have internal constructors, and can only be constructed by an API call (creating a new Address, retrieving an existing Address). This ensures that the object does in fact exist server-side, and returns a read-only copy of the data locally.

With that said, we are in discussions of enhancing our data flow methods, including improving the way a user can pass in parameters to an API call. For example, a "create address" function call would accept an "AddressParameters" object, which could be constructed using the typical C# constructor pattern (see our Go library for an example: https://github.com/EasyPost/easypost-go/blob/master/shipment.go#L188). As you point out, the Extensions library (made by yours truly) is a proof-of-concept of this and can be used as a temporary workaround until first-party support for this process is adopted.

willsmith9182 commented 1 year ago

This is a very disappointing response. Not being able to create instance of objects makes any form of unit testing a pain. As has already been pointed out, using dictionaries to fill an object type is completely backwards.

to avoid a potentially confusing local-vs-server-side data mismatch

I want the ability to do this, so I don't have to call your api in a unit test just to create an instance of a public model.

bnbry commented 1 year ago

We've heard the feedback and are working on bringing this back in line with expectations. Thanks everyone for sharing your views and helping us understand it from a users perspective. We'll update this ticket as we make progress on a solution.

nwithan8 commented 1 year ago

Hello everyone, we just wanted to let you all know that we have begun work on a solution to use objects, rather than dictionaries, to craft parameter sets for API calls. Feel free to check out #409 and leave us any feedback on whether this will help your EasyPost integration!

nwithan8 commented 1 year ago

Hello everyone, wanted to let you know that we have implemented "parameter sets" in this library in our most recent release.

These utilize classes rather than dictionaries to help you construct parameters for any given API call. There are also overrides to each method that accepts these new parameter sets rather than dictionaries. This should hopefully help you not only discover what parameters are available/required for each API call/function, but also prevent issues such as spelling errors, incorrect types and invalid schemas (that's all handled behind the scenes by the library).

This feature is currently in beta, and can be accessed in the EasyPost.BetaFeatures.Parameters namespace (more information in the README). Please let us know how these work out for you all!

I'm going to go ahead and close this issue, but please open a new one if any issues arise! Thanks for the feedback!