From 75bbf10e1fbe283a1d823e30d55c30c39e1d3187 Mon Sep 17 00:00:00 2001 From: Andreas Gullberg Larsen Date: Thu, 2 Jan 2025 11:50:16 +0100 Subject: [PATCH] =?UTF-8?q?=F0=9F=9A=B8Describe=20why=20Parse=20fails=20du?= =?UTF-8?q?e=20to=20case-insensitive=20ambiguity=20(#1484)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fixes #1423 `UnitParser.Parse("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 --- .../AbbreviatedUnitsConverter.cs | 2 +- UnitsNet.Tests/QuantityParserTests.cs | 5 ++-- UnitsNet.Tests/UnitParserTests.cs | 11 ++++++-- UnitsNet/CustomCode/UnitParser.cs | 25 +++++++++++-------- 4 files changed, 28 insertions(+), 15 deletions(-) diff --git a/UnitsNet.Serialization.JsonNet/AbbreviatedUnitsConverter.cs b/UnitsNet.Serialization.JsonNet/AbbreviatedUnitsConverter.cs index 79c6e4fd58..97cebf9eb9 100644 --- a/UnitsNet.Serialization.JsonNet/AbbreviatedUnitsConverter.cs +++ b/UnitsNet.Serialization.JsonNet/AbbreviatedUnitsConverter.cs @@ -178,7 +178,7 @@ protected string GetQuantityType(IQuantity quantity) } /// - /// 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. /// /// An exhaustive search using all quantities is very likely to fail with an /// , so make sure you're using the minimum set of supported quantities. diff --git a/UnitsNet.Tests/QuantityParserTests.cs b/UnitsNet.Tests/QuantityParserTests.cs index c4d00f2558..b56fa10d90 100644 --- a/UnitsNet.Tests/QuantityParserTests.cs +++ b/UnitsNet.Tests/QuantityParserTests.cs @@ -42,7 +42,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"); @@ -54,7 +54,8 @@ void Act() quantityParser.Parse("1 Foo", null, (value, unit) => new HowMuch((double) value, unit)); } - Assert.Throws(Act); + var ex = Assert.Throws(Act); + Assert.Equal("""Cannot parse "Foo" since it matches multiple units: ATon ("FOO"), Some ("foo").""", ex.Message); } [Fact] diff --git a/UnitsNet.Tests/UnitParserTests.cs b/UnitsNet.Tests/UnitParserTests.cs index 309afe5df7..50652a8963 100644 --- a/UnitsNet.Tests/UnitParserTests.cs +++ b/UnitsNet.Tests/UnitParserTests.cs @@ -115,8 +115,8 @@ public void Parse_AmbiguousUnitsThrowsException() var exception2 = Assert.Throws(() => Length.Parse("1 pt", CultureInfo.InvariantCulture)); // Assert - Assert.Equal("Cannot parse \"pt\" since it could be either of these: DtpPoint, PrinterPoint", exception1.Message); - Assert.Equal("Cannot parse \"pt\" since it could be either of these: DtpPoint, PrinterPoint", exception2.Message); + Assert.Equal("""Cannot parse "pt" since it matches multiple units: DtpPoint ("pt"), PrinterPoint ("pt").""", exception1.Message); + Assert.Equal("""Cannot parse "pt" since it matches multiple units: DtpPoint ("pt"), PrinterPoint ("pt").""", exception2.Message); } [Theory] @@ -144,6 +144,13 @@ public void Parse_MappedCustomUnit() Assert.Equal(HowMuchUnit.Some, parsedUnit); } + [Fact] + public void Parse_LengthUnit_MM_ThrowsExceptionDescribingTheAmbiguity() + { + var ex = Assert.Throws(() => UnitsNetSetup.Default.UnitParser.Parse("MM")); + Assert.Equal("""Cannot parse "MM" since it matches multiple units: Megameter ("Mm"), Millimeter ("mm").""", ex.Message); + } + [Fact] public void TryParse_WithNullAbbreviation_ReturnsFalse() { diff --git a/UnitsNet/CustomCode/UnitParser.cs b/UnitsNet/CustomCode/UnitParser.cs index e55fdaf2b6..77f96b1018 100644 --- a/UnitsNet/CustomCode/UnitParser.cs +++ b/UnitsNet/CustomCode/UnitParser.cs @@ -87,8 +87,8 @@ public Enum Parse(string unitAbbreviation, Type unitType, IFormatProvider? forma formatProvider = UnitAbbreviationsCache.FallbackCulture; continue; default: - var unitsCsv = string.Join(", ", matches.Select(x => Enum.GetName(unitType, x.Unit)).ToArray()); - throw new AmbiguousUnitParseException($"Cannot parse \"{unitAbbreviation}\" since it could be either of these: {unitsCsv}"); + var unitsCsv = string.Join(", ", matches.Select(x => $"{Enum.GetName(unitType, x.Unit)} (\"{x.Abbreviation}\")").OrderBy(x => x)); + throw new AmbiguousUnitParseException($"Cannot parse \"{unitAbbreviation}\" since it matches multiple units: {unitsCsv}."); } } } @@ -216,21 +216,26 @@ public bool TryParse([NotNullWhen(true)] string? unitAbbreviation, Type unitType // TODO see about optimizing this method: both Parse and TryParse only care about having one match (in case of a failure we could return the number of matches) List<(Enum Unit, string Abbreviation)> stringUnitPairs = _unitAbbreviationsCache.GetStringUnitPairs(enumValues, formatProvider); (Enum Unit, string Abbreviation)[] matches = - stringUnitPairs.Where(pair => pair.Item2.Equals(unitAbbreviation, StringComparison.OrdinalIgnoreCase)).ToArray(); + stringUnitPairs.Where(pair => pair.Abbreviation.Equals(unitAbbreviation, StringComparison.OrdinalIgnoreCase)).ToArray(); if (matches.Length == 0) { - unitAbbreviation = NormalizeUnitString(unitAbbreviation); - matches = stringUnitPairs.Where(pair => pair.Item2.Equals(unitAbbreviation, StringComparison.OrdinalIgnoreCase)).ToArray(); + var normalizeUnitString = NormalizeUnitString(unitAbbreviation); + if (unitAbbreviation != normalizeUnitString) + { + unitAbbreviation = normalizeUnitString; + matches = stringUnitPairs.Where(pair => pair.Abbreviation.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) + if (matches.Length == 1) { - matches = stringUnitPairs.Where(pair => pair.Item2.Equals(unitAbbreviation)).ToArray(); + return matches; } - - return matches; + + // Narrow the search if too many hits, for example Megabar "Mbar" and Millibar "mbar" need to be distinguished + (Enum Unit, string Abbreviation)[] caseSensitiveMatches = stringUnitPairs.Where(pair => pair.Abbreviation.Equals(unitAbbreviation)).ToArray(); + return caseSensitiveMatches.Length == 0 ? matches : caseSensitiveMatches; } } }