-
-
Notifications
You must be signed in to change notification settings - Fork 313
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
3.0 remove enums and replace them with strings. #65
Conversation
Todo:
|
/// </summary> | ||
[JsonProperty("status")] | ||
public ShopifyOrderStatus? Status { get; set; } = ShopifyOrderStatus.Any; | ||
public string Status { get; set; } = "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.
Maybe not default this to "any", as if the developer forgets to populate it, pulling any can be dangerous as it would pull all orders. with some of the shops I've been testing with I've found not all of them Archive their orders because they like visibility of them - defaulting this could cause hundreds of thousands of orders to potentially be pulled
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.
Good point, thanks!
All tests are passing. |
It would be useful to have string constants in addition to the documentation. You may also be able to reduce breaking existing code if you specify the constants like the enums were, ex:
|
Isn't the point of removing the enums to your ass control back to the developer? Even if string constants were in place, we'd still not be able to react quickly to a shopify interface change |
That's a pretty elegant solution, @MalikDrako. I'm disappointed that I hadn't considered it before releasing 3.0! @ZubyH, this would let us continue to use strings instead of enums as we do now in 3.0, but we'd also be able to use "hardcoded" strings so we don't have to remember the exact values that need to be sent. Then if Shopify were to change a value you could still quickly update your app to use the new strings instead of waiting for an update from ShopifySharp. It's essentially having a class of static strings that we can use to remember the currently known values. string status = ShopifyFulfillmentStatus.Fulfilled; // This is now a static string instead of an enum
var filter = new ShopifyFulfillmentFilter()
{
status = status
}; Then if Shopify were to change the values of Fulfillment statuses, you could quickly update your app without waiting for an update from ShopifySharp because the filter still accepts strings: string status = "updated_value";
var filter = new ShopifyFulfillmentFilter()
{
status = status
} |
This 3.0 release is a major SemVer release, containing a breaking change by removing almost all enums and changing their usage across various classes to a string property.
Why did we remove enums?
I'm a big fan of using enums to make things easier for C# devs, because it removes a lot of the headache that comes with trying to remember all the valid string options for certain properties. With enums, we get those options hardcoded by default. We can easily scroll up and down the list of known values and select the one we need, without having to worry about typos.
Many Shopify objects have string properties that only accept a predetermined list of values, and these properties are perfect for matching to C# enums. Unfortunately, Shopify has a habit of only documenting the most used values and leaving the developer to guess the rest. On top of that, they sometimes change those enums completely, such as this case where they changed the enums used for filtering orders without announcing it.
That's a problem when it comes to strongly-typed languages like C#. If you receive an enum property that doesn't have a value matching the enum, you're going to get a big fat exception thrown in your face. This is especially problematic when these undocumented enum values are sent to you automatically in webhooks.
On top of that, if there's an enum value that you need to send but isn't in ShopifySharp, you'll need to wait until a new version of the lib is released before you can use it.
Enums would be much better suited to ShopifySharp if Shopify themselves used API versioning, but sadly that isn't the case. After struggling with undocumented values and unannounced changes that break apps through two major releases of ShopifySharp, I've made the decision to pull the plug on almost all enums in the lib.
What were previously enums in ShopifySharp 1.x and 2.x are now string properties. This change will prevent breaking your app when an enum value changes, and will allow you to quickly update your app when a new enum value is released without waiting on an update to ShopifySharp first.
This will close #64 when merged.