Google.Type.Money <=> decimal

[nycdotnet]

Greetings,

I am putting together a PR to add an extension method on decimal to convert to Google.Type.Money with an optional currency code, and to add a ToDecimal() extension on Google.Type.Money that omits the currency type. The pattern used will be similar to #251 and #256.

I have a few questions that I'd love to resolve ahead of time.

1. Are you ok with updating xunit to the latest stable 2.3.1? This solution currently uses a beta of 2.3.0 which we find is a bit buggy, particularly with regards to the VS runner.
2. I am currently using public static Money ToMoney(this decimal value, string currencyCode = "") as the signature of the ToMoney extension method. Is this acceptable to you? Would it be better to use default with currencyCode which would require upgrading the project from C# 7 to C# 7.1?
3. What do you think the behavior of ToMoney() should be in cases where the decimal value does not fit? (see below).

I am thinking that if the integral part of the decimal doesn't fit in the Units field (long), the OverflowException should be caught and used as the InnerException of an ArgumentOutOfRangeException. Would love your thoughts on if there is value in doing this, or if we should just not catch the OverflowException.

What should happen when the Nanos don't fit is less clear, however. For example, should 0.555_555_555_5M be truncated to 555_555_555 nanos? rounded to 555_555_556 nanos? Should this throw? Should the user be able to specify using some sort of enum with the default being to truncate since this sort of thing is only relevant to a very small number of people who will likely want to specify the behavior?

Would love your thoughts on the above. Thanks!

[jskeet]

Hi Steve,

I'm currently flying back from a conference, and will be at more conferences next week. I'll try to give this some thought, but it may be a few days. Hope that's okay!

[nycdotnet]

No worries at all. Thanks and safe travels.

[jskeet]

1. I'm fine with the xunit update

2. Using the default literal wouldn't work, as the default for string is null, which would immediately fail validation. I'm not sure that it's a good idea to have a default at all, to be honest. If you're creating a Money value, you should provide the currency. I'd expect anything receiving a value to want there to be a currency code. (Validating that it's a suitable ISO 4217 might be a bit far, admittedly.)

3. ArgumentOutOfRangeException sounds reasonable if the units don't fit. I would err on the side of throwing if the nanos don't fit as well - but you could potentially have an overload accepting a RoundingMode enum or something like that. I'd be tempted to add that at a later time though - which is another argument in favour of not having a default value for the currency parameter, as overloading gets hairier with optional parameters.

cc @chrisdunelm for visibility - Chris, does the above all sound reasonable to you?

[chrisdunelm]

1. Agreed
2. I haven't thought this through much, but I also would expect that a currency must be provided; so no default, and the method should validate that the currency provided is not empty. Agreed that validating to ISO4217 is not necessary.
3. I also think it'd be best to throw if the nanos don't fit. Ensuring round-tripping of this value seems like a very good idea. Also agree that a method with a RoundingMode could be added.

[nycdotnet]

Thanks. I will make the ToMoney extension method have a required string currencyCode on it and throw ArgumentOutOfRange if either the units or nanos cannot be represented exactly (aka are not round-trippable). I prefer to have passing "" as the currencyCode not throw, as specifying "" when creating the proto manually with "" does not throw today. Attempting to set null on the CurrencyCode field does already throw, and the extension method will inherit this behavior. I agree the rounding stuff can be done at a later time if anyone is so interested. Thanks!

[jskeet]

Implemented via #267.

Closing this PR - but please let me know the level of urgency for creating a release with this change in. (I have no explicit reason to wait, beyond "it's a little bit of work to do it".)

[nycdotnet]

No urgency. Thank you.