Remove reflection #618
Remove reflection #618
Conversation
| @@ -77,13 +77,6 @@ public void ConvertByAbbreviation_ConvertsTheValueToGivenUnit(double expectedVal | |||
| Assert.Equal(expectedValue, UnitConverter.ConvertByAbbreviation(inputValue, quantityTypeName, fromUnit, toUnit)); | |||
| } | |||
|
|
|||
| [Theory] | |||
| [InlineData(1, "UnknownQuantity", "m", "cm")] | |||
| public void ConvertByAbbreviation_ThrowsQuantityNotFoundExceptionOnUnknownQuantity(double inputValue, string quantityTypeName, string fromUnit, string toUnit) | |||
There was a problem hiding this comment.
Hm, why was this test case removed? Is it no longer relevant/valid?
There was a problem hiding this comment.
I see further down why, but I think we can keep the test case and test for UnitNotFoundException instead?
There was a problem hiding this comment.
It fails because it doesn't throw it any more 😃 Code is unnecessary.
There was a problem hiding this comment.
But.. I honestly don't know what result to expect when calling this method with these arguments? What results are we getting if not exception?
There was a problem hiding this comment.
It will also give UnitNotFoundException. I'll add it back and catch that.
|
|
||
| return Quantity.From(value, unitValue); | ||
| } | ||
|
|
||
| private static object TryDeserializeIComparable(JsonReader reader, JsonSerializer serializer) | ||
| { | ||
| var token = JToken.Load(reader); | ||
| JToken token = JToken.Load(reader); |
There was a problem hiding this comment.
I see you reverted all the types back. That's fine, although I only meant for the few places where the exact type wasn't obvious to have explicit types.
|
Very nice 🎉 |
IQuantity and QuantityInfo negates the need for a lot of reflection 👍