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

Price (and other monetary properties) in models should not be type double #74

Closed
ghost opened this issue Sep 28, 2016 · 5 comments
Closed
Labels
Milestone

Comments

@ghost
Copy link

ghost commented Sep 28, 2016

It's generally a very-very bad idea to save money (prices, etc) in double type. There's a decimal C# type just for mostly monetary purposes that won't give you calculation, casting, rounding, comparing mistakes that real types will give from time to time.
From what I see it's classes:

  • ShopifyCharge,
  • ShopifyProductVariant,
  • ShopifyCustomer,
  • ShopifyLineItem,
  • ShopifyOrder,
  • ShopifyRecurringCharge,
  • ShopifyShippingLine,
  • ShopifyTaxLine,
  • ShopifyTransaction,
  • ShopifyUsageCharge

In case you have other things to do I can fix it on this or next weekend and pull-request.

@nozzlegear
Copy link
Owner

Thanks for reporting this! I tend to get doubles and decimals confused, I don't use them much in anything I've built for production. A pull request would certainly be welcome, otherwise I'll get it when I have the chance.

@thj-dk
Copy link
Contributor

thj-dk commented Nov 14, 2016

Decimals are definately the most appropriate type for this. This this might create breaking changes for some. What do you think @nozzlegear?

@nozzlegear
Copy link
Owner

I would agree that changing these types to decimal is probably a breaking change, and would warrant waiting until 4.0 for the change to make it in.

@clement911
Copy link
Collaborator

@nozzlegear how can I contribute to 4.0?
Should you create a branch?

@nozzlegear nozzlegear added this to the 4.0 milestone Jan 16, 2017
@nozzlegear
Copy link
Owner

All doubles have been change to decimals in the 4.0 branch.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants