Skip to content

Commit

Permalink
🚸Describe why Parse fails due to case-insensitive ambiguity (#1484)
Browse files Browse the repository at this point in the history
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 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
  • Loading branch information
angularsen authored Jan 2, 2025
1 parent bb1420e commit f814884
Show file tree
Hide file tree
Showing 4 changed files with 28 additions and 7 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -190,7 +190,7 @@ protected string GetQuantityType(IQuantity quantity)
}

/// <summary>
/// Attempt to find an a unique (non-ambiguous) unit matching the provided abbreviation.
/// Attempt to find a unique (non-ambiguous) unit matching the provided abbreviation.
/// <remarks>
/// An exhaustive search using all quantities is very likely to fail with an
/// <exception cref="AmbiguousUnitParseException" />, so make sure you're using the minimum set of supported quantities.
Expand Down
7 changes: 4 additions & 3 deletions UnitsNet.Tests/QuantityParserTests.cs
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
// Licensed under MIT No Attribution, see LICENSE file at the root.
// Licensed under MIT No Attribution, see LICENSE file at the root.
// Copyright 2013 Andreas Gullberg Larsen (andreas.larsen84@gmail.com). Maintained at https://github.com/angularsen/UnitsNet.

using UnitsNet.Tests.CustomQuantities;
Expand Down Expand Up @@ -40,7 +40,7 @@ public void Parse_WithOneCaseInsensitiveMatchAndOneExactMatch_ParsesWithTheExact
}

[Fact]
public void Parse_WithMultipleCaseInsensitiveMatchesButNoExactMatches_ThrowsUnitNotFoundException()
public void Parse_WithMultipleCaseInsensitiveMatchesButNoExactMatches_ThrowsAmbiguousUnitParseException()
{
var unitAbbreviationsCache = new UnitAbbreviationsCache();
unitAbbreviationsCache.MapUnitToAbbreviation(HowMuchUnit.Some, "foo");
Expand All @@ -52,7 +52,8 @@ void Act()
quantityParser.Parse<HowMuch, HowMuchUnit>("1 Foo", null, (value, unit) => new HowMuch((double) value, unit));
}

Assert.Throws<UnitNotFoundException>(Act);
var ex = Assert.Throws<AmbiguousUnitParseException>(Act);
Assert.Equal("Cannot parse \"Foo\" since it matched multiple units [Some, ATon] with case-insensitive comparison, but zero units with case-sensitive comparison. To resolve the ambiguity, pass a unit abbreviation with the correct casing.", ex.Message);
}

[Fact]
Expand Down
8 changes: 8 additions & 0 deletions UnitsNet.Tests/UnitParserTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -136,5 +136,13 @@ public void Parse_MappedCustomUnit()

Assert.Equal(HowMuchUnit.Some, parsedUnit);
}

[Fact]
public void Parse_LengthUnit_MM_ThrowsExceptionDescribingTheAmbiguity()
{
var ex = Assert.Throws<AmbiguousUnitParseException>(() => UnitsNetSetup.Default.UnitParser.Parse<LengthUnit>("MM"));
Assert.Contains("Cannot parse \"MM\" since it matched multiple units [Millimeter, Megameter] with case-insensitive comparison, but zero units with case-sensitive comparison. To resolve the ambiguity, pass a unit abbreviation with the correct casing.", ex.Message);
}

}
}
18 changes: 15 additions & 3 deletions UnitsNet/CustomCode/UnitParser.cs
Original file line number Diff line number Diff line change
Expand Up @@ -72,14 +72,19 @@ public Enum Parse(string? unitAbbreviation, Type unitType, IFormatProvider? form
var stringUnitPairs = _unitAbbreviationsCache.GetStringUnitPairs(enumValues, formatProvider);
var matches = stringUnitPairs.Where(pair => pair.Item1.Equals(unitAbbreviation, StringComparison.OrdinalIgnoreCase)).ToArray();

// No match? Retry after normalizing the unit abbreviation.
if(matches.Length == 0)
{
unitAbbreviation = NormalizeUnitString(unitAbbreviation);
matches = stringUnitPairs.Where(pair => pair.Item1.Equals(unitAbbreviation, StringComparison.OrdinalIgnoreCase)).ToArray();
}

// Narrow the search if too many hits, for example Megabar "Mbar" and Millibar "mbar" need to be distinguished
if(matches.Length > 1)
var caseInsensitiveMatches = matches;

// More than one case-insensitive match? Retry with case-sensitive match.
// For example, Megabar "Mbar" and Millibar "mbar" need to be distinguished.
bool hasMultipleCaseInsensitiveMatches = matches.Length > 1;
if (hasMultipleCaseInsensitiveMatches)
matches = stringUnitPairs.Where(pair => pair.Item1.Equals(unitAbbreviation)).ToArray();

switch(matches.Length)
Expand All @@ -88,11 +93,18 @@ public Enum Parse(string? unitAbbreviation, Type unitType, IFormatProvider? form
return (Enum)Enum.ToObject(unitType, matches[0].Unit);
case 0:
// Retry with fallback culture, if different.
if(!Equals(formatProvider, UnitAbbreviationsCache.FallbackCulture))
if (formatProvider != null && !Equals(formatProvider, UnitAbbreviationsCache.FallbackCulture))
{
return Parse(unitAbbreviation, unitType, UnitAbbreviationsCache.FallbackCulture);
}

if (hasMultipleCaseInsensitiveMatches)
{
string ciUnitsCsv = string.Join(", ", caseInsensitiveMatches.Select(x => Enum.GetName(unitType, x.Unit)));
throw new AmbiguousUnitParseException(
$"Cannot parse \"{unitAbbreviation}\" since it matched multiple units [{ciUnitsCsv}] with case-insensitive comparison, but zero units with case-sensitive comparison. To resolve the ambiguity, pass a unit abbreviation with the correct casing.");
}

throw new UnitNotFoundException($"Unit not found with abbreviation [{unitAbbreviation}] for unit type [{unitType}].");
default:
string unitsCsv = string.Join(", ", matches.Select(x => Enum.GetName(unitType, x.Unit)).ToArray());
Expand Down

0 comments on commit f814884

Please sign in to comment.