Amount and Currency refactor #44
Conversation
…ndard 2.0 which the amount data type uses to add context to the decimal value. Removed float and int constrcutors to reduce internal rounding issues.
…ad also fail if amount does not contain a currency?
|
Can you exclude the sln file? |
|
Updated most changes as requested. Do i need to create a new pull request? not sure how this works. |
|
You add commits on top of your fork branch and it adds it on the PR. Simple as that 😄 |
|
ok thanks. hope this round goes better. Should the MoneyDataType library be made into an orchard module so others can benefit from the work too? I am trying to write another module which needs money/currencies too and it would be good to leverage the same feature. Just not sure how to go about it and what it would need extra. Some guidance in that department would be good if that is the path we will take. |
|
I really appreciate your patience and contribution. It's fairly normal for a first PR to a project to be generating a lot of feedback :) This is good stuff. So about MoneyDataType, to me it's not an Orchard module, it's a library, useful independently of Orchard, and taking no dependency on Orchard. So the way to make it available to others, I think, is to package it in NuGet form. That means adding the necessary metadata to the project file (super-easy through the project properties dialog in VS, or by editing the project's XML), and then publishing the package on NuGet.org. Long-term, we could move the code to a separate repo, or keep it here and continue to reference it as a project ref, either way is fine. Maybe keeping it around is a little more convenient for us, but then again I don't expect it to change a lot going forward. I'm looking at your new changes and comments now. |
…turn "foreign" currencies that don't appear in default table. Updated IsoCode name to CurrentIsoCode where appropriate. Introduced DefaultProvider internal property in Currency. Fixed bug in CurrencyProvider whereby items not in dictionary would throw.
Ensure Unknown currencies can be serialised and deserialised Changed Currency type to be struct for value comparison instead of reference comparison to aid serialisation.
|
Finally updated to support unknown currencies. I think this solves the issue of 2 step resolution of currencies. basically it alters the way a currency is serialised depending on the new |
|
Cool, thanks for the changes! I'm going to merge that in a few minutes unless there's anything more you want to do first. |
|
All good, and thanks for all the great feedback. Really enjoyed the process, and I think we have a solid library to build on. |
Major refactor to introduce Currencies as a built-in list from NetStandard 2.0 which the amount data type uses to add context to the decimal value.
Don't make any assumptions when writing serialized amounts. Should read also fail if amount does not contain a currency?
Should the money project be made a module? I'm very new to Orchard so not sure. I am attempting to create a module too which uses money amounts and it would be good to standardize the money library.