🚸Describe why Parse fails due to case-insensitive ambiguity#1484
🚸Describe why Parse fails due to case-insensitive ambiguity#1484angularsen merged 2 commits intomasterfrom
Conversation
Fixes #1423 `UnitParser.Parse<LengthUnit>("MM")` fails due to matching both `Megameter` and `Millimeter` in case-insensitive matching, but matches neither of them in the case-sensitive fallback. It was confusing to get `UnitsNotFoundException` in this case, since case-insensitive usually works for most untis. ### Changes - Handle this case and throw `AmbiguousUnitParseException` instead of `UnitNotFoundException` - Describe the case-insensitive units that matched
|
@lipchev relevant to your exception helpers PR, the exception message can be improved further to include information about the quantity and unit enum type, which I believe you already did there. |
Yes, I was also going to bring this is up (eventually) - if we want to go wild we could even include extra fields like in these examples: throw new UnitNotFoundException($"No unit was found for quantity '{quantityName}' with the name: '{unitName}'.")
{
Data = { ["quantityName"] = quantityName, ["unitName"] = unitName }
}; throw new UnitNotFoundException($"No quantity was found with the specified unit type: '{unitType}'.") { Data = { ["unitType"] = unitType.Name } }; |
…rowsUnitNotFoundException
|
Fixed test case that broken in CI, due to different current culture: def3211 |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #1484 +/- ##
======================================
Coverage 84% 84%
======================================
Files 368 368
Lines 35601 35606 +5
======================================
+ Hits 30000 30005 +5
Misses 5601 5601 ☔ View full report in Codecov by Sentry. |
|
@lipchev Are you still reviewing this or can I merge? |
Well, my earlier comment wasn't about the test that broke the build - it was a bit more subtle than that.. |
|
Gotcha, I'll merge this then. |
…en#1484) Fixes angularsen#1423 `UnitParser.Parse<LengthUnit>("MM")` fails due to matching both `Megameter` and `Millimeter` in case-insensitive matching, but matches neither of them in the case-sensitive fallback. It was confusing to get `UnitsNotFoundException` in this case, since case-insensitive usually works for most units. ### Changes - Handle this case and throw `AmbiguousUnitParseException` instead of `UnitNotFoundException` - Describe the case-insensitive units that matched - Fix existing test `Parse_WithMultipleCaseInsensitiveMatchesButNoExactMatches_ThrowsUnitNotFoundException` - Skip retrying with fallback culture if no specific `formatProvider` was given
… UnitNotFoundException (#1484) (#1494) Fixes #1423 for `release-v6` Carry over from #1484 `UnitParser.Parse<LengthUnit>("MM")` fails due to matching both `Megameter` and `Millimeter` in case-insensitive matching, but matches neither of them in the case-sensitive fallback. It was confusing to get `UnitsNotFoundException` in this case, since case-insensitive usually works for most units. ### Changes - Handle this case and throw `AmbiguousUnitParseException` instead of `UnitNotFoundException` - Describe the case-insensitive units that matched - Fix existing test `Parse_WithMultipleCaseInsensitiveMatchesButNoExactMatches_ThrowsUnitNotFoundException` - Skip retrying with fallback culture if no specific `formatProvider` was given - Avoid some of the unnecessary array allocations Co-authored-by: Andreas Gullberg Larsen <[email protected]>
2 participants
Fixes #1423
UnitParser.Parse<LengthUnit>("MM")fails due to matching bothMegameterandMillimeterin case-insensitive matching, but matches neither of them in the case-sensitive fallback. It was confusing to getUnitsNotFoundExceptionin this case, since case-insensitive usually works for most units.Changes
AmbiguousUnitParseExceptioninstead ofUnitNotFoundExceptionParse_WithMultipleCaseInsensitiveMatchesButNoExactMatches_ThrowsUnitNotFoundExceptionformatProviderwas given