Skip to content

Fix #1443: UnitParser.TryParse doesn't try with FallbackCulture (v6) #1466

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
angularsen merged 3 commits into from
Dec 26, 2024
Merged

Fix #1443: UnitParser.TryParse doesn't try with FallbackCulture (v6) #1466

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
ea3a23f
Fix #1443: UnitParser.TryParse doesn't try with FallbackCulture
lipchev Dec 19, 2024
dcd4fb0
Improve the test coverage for the static ParseUnit and TryParseUnit t…
lipchev Dec 19, 2024
70f1b3e
Add/improve test cases
angularsen Dec 26, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
41 changes: 40 additions & 1 deletion UnitsNet.Tests/UnitParserTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,12 @@ public void Parse_GivenAbbreviationsThatAreAmbiguousWhenLowerCase_ReturnsCorrect
Assert.Equal(PressureUnit.Millibar, Pressure.ParseUnit("mbar"));
}

[Fact]
public void Parse_NullAbbreviation_Throws_ArgumentNullException()
{
Assert.Throws<ArgumentNullException>(() => UnitsNetSetup.Default.UnitParser.Parse(null!, typeof(LengthUnit)));
}

[Fact]
public void Parse_UnknownAbbreviationThrowsUnitNotFoundException()
{
Expand Down Expand Up @@ -95,7 +101,7 @@ public void Parse_CanParseMultiplySigns(string unitAbbreviation, Type unitType,
[InlineData("kg·s⁻¹·m⁻² ", typeof(MassFluxUnit), MassFluxUnit.KilogramPerSecondPerSquareMeter)]
[InlineData("k g · s ⁻ ¹ · m ⁻ ² ", typeof(MassFluxUnit), MassFluxUnit.KilogramPerSecondPerSquareMeter)]
[InlineData(" k g · s ⁻ ¹ · m ⁻ ² ", typeof(MassFluxUnit), MassFluxUnit.KilogramPerSecondPerSquareMeter)]
public void Parse_CanParseWithWithspacesInUnit(string unitAbbreviation, Type unitType, Enum resultUnitType)
public void Parse_CanParseWithWhitespacesInUnit(string unitAbbreviation, Type unitType, Enum resultUnitType)
{
Assert.Equal(resultUnitType, UnitParser.Default.Parse(unitAbbreviation, unitType));
}
Expand All @@ -121,6 +127,7 @@ public void Parse_AmbiguousUnitsThrowsException()
[InlineData("г", "ru-RU", MassUnit.Gram)]
[InlineData("kg", "en-US", MassUnit.Kilogram)]
[InlineData("кг", "ru-RU", MassUnit.Kilogram)]
[InlineData("kg", "ru-RU", MassUnit.Kilogram)] // should work with the "FallbackCulture"
Copy link
Collaborator Author

@lipchev lipchev Dec 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was passing before (but was never tested).

public void ParseMassUnit_GivenCulture(string str, string cultureName, Enum expectedUnit)
{
Assert.Equal(expectedUnit, UnitParser.Default.Parse<MassUnit>(str, CultureInfo.GetCultureInfo(cultureName)));
Expand Down Expand Up @@ -152,5 +159,37 @@ public void TryParse_WithNullAbbreviation_ReturnsFalse()
Assert.False(success);
});
}

[Fact]
public void TryParse_UnknownAbbreviation_ReturnsFalse()
{
Assert.False(UnitsNetSetup.Default.UnitParser.TryParse("nonexistingunit", out AreaUnit _));
}

[Fact]
public void TryParse_WithAmbiguousUnits_ReturnsFalse()
{
UnitParser unitParser = UnitsNetSetup.Default.UnitParser;
Assert.False(unitParser.TryParse("pt", CultureInfo.InvariantCulture, out LengthUnit _));
}

[Theory]
[InlineData("ng", "en-US", MassUnit.Nanogram)]
[InlineData("нг", "ru-RU", MassUnit.Nanogram)]
[InlineData("g", "en-US", MassUnit.Gram)]
[InlineData("г", "ru-RU", MassUnit.Gram)]
[InlineData("kg", "en-US", MassUnit.Kilogram)]
[InlineData("кг", "ru-RU", MassUnit.Kilogram)]
[InlineData("kg", "ru-RU", MassUnit.Kilogram)] // should work with the "FallbackCulture"
Copy link
Collaborator Author

@lipchev lipchev Dec 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was failing before (the issue mentioned in #1443)

public void TryParseMassUnit_GivenCulture(string str, string cultureName, Enum expectedUnit)
{
var formatProvider = CultureInfo.GetCultureInfo(cultureName);
UnitParser unitParser = UnitsNetSetup.Default.UnitParser;

var success = unitParser.TryParse(str, formatProvider, out MassUnit unitParsed);

Assert.True(success);
Assert.Equal(expectedUnit, unitParsed);
}
}
}
8 changes: 4 additions & 4 deletions UnitsNet/CustomCode/UnitAbbreviationsCache.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -271,12 +271,12 @@ public IReadOnlyList<string> GetAllUnitAbbreviationsForQuantity(Type unitEnumTyp
{
var enumValues = Enum.GetValues(unitEnumType).Cast<Enum>();
var all = GetStringUnitPairs(enumValues, formatProvider);
return all.Select(pair => pair.Item1).ToList();
return all.Select(pair => pair.Item2).ToList();
}

internal List<(string Abbreviation, Enum Unit)> GetStringUnitPairs(IEnumerable<Enum> enumValues, IFormatProvider? formatProvider = null)
internal List<(Enum Unit, string Abbreviation)> GetStringUnitPairs(IEnumerable<Enum> enumValues, IFormatProvider? formatProvider = null)
{
var ret = new List<(string, Enum)>();
var ret = new List<(Enum, string)>();
formatProvider ??= CultureInfo.CurrentCulture;

foreach(var enumValue in enumValues)
Expand All @@ -285,7 +285,7 @@ public IReadOnlyList<string> GetAllUnitAbbreviationsForQuantity(Type unitEnumTyp
{
foreach(var abbrev in abbreviations)
{
ret.Add((abbrev, enumValue));
ret.Add((enumValue, abbrev));
}
}
}
Expand Down
112 changes: 65 additions & 47 deletions UnitsNet/CustomCode/UnitParser.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
// Copyright 2013 Andreas Gullberg Larsen ([email protected]). Maintained at https://github.com/angularsen/UnitsNet.

using System;
using System.Collections.Generic;
using System.Diagnostics.CodeAnalysis;
using System.Globalization;
using System.Linq;
Expand Down Expand Up @@ -44,7 +45,8 @@ public UnitParser(UnitAbbreviationsCache? unitAbbreviationsCache)
/// <param name="formatProvider">The format provider to use for lookup. Defaults to <see cref="CultureInfo.CurrentCulture" /> if null.</param>
/// <typeparam name="TUnitType"></typeparam>
/// <returns></returns>
public TUnitType Parse<TUnitType>(string unitAbbreviation, IFormatProvider? formatProvider = null) where TUnitType : Enum
public TUnitType Parse<TUnitType>(string unitAbbreviation, IFormatProvider? formatProvider = null)
where TUnitType : Enum
Copy link
Collaborator Author

@lipchev lipchev Dec 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is likely going to annoy some people (a little) but I really think we should make all Enum constraints have the struct, Enum - this makes sure that we're only calling this with a concrete type, disabling the potential call:
Enum whatType = UnitParser.Parse<Enum>(abbreviation, null);

Copy link
Owner

@angularsen angularsen Dec 26, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see why not, I think it was a mistake to not restrict to struct on all methods. I see only some of them have it.

I'll push that as a separate PR.

{
return (TUnitType)Parse(unitAbbreviation, typeof(TUnitType), formatProvider);
}
Expand All @@ -66,37 +68,27 @@ public Enum Parse(string unitAbbreviation, Type unitType, IFormatProvider? forma
{
if (unitAbbreviation == null) throw new ArgumentNullException(nameof(unitAbbreviation));
unitAbbreviation = unitAbbreviation.Trim();

var enumValues = Enum.GetValues(unitType).Cast<Enum>();
var stringUnitPairs = _unitAbbreviationsCache.GetStringUnitPairs(enumValues, formatProvider);
var matches = stringUnitPairs.Where(pair => pair.Item1.Equals(unitAbbreviation, StringComparison.OrdinalIgnoreCase)).ToArray();

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)
matches = stringUnitPairs.Where(pair => pair.Item1.Equals(unitAbbreviation)).ToArray();

switch(matches.Length)
Enum[] enumValues = Enum.GetValues(unitType).Cast<Enum>().ToArray();
while (true)
{
case 1:
return (Enum)Enum.ToObject(unitType, matches[0].Unit);
case 0:
// Retry with fallback culture, if different.
if(!Equals(formatProvider, UnitAbbreviationsCache.FallbackCulture))
{
return Parse(unitAbbreviation, unitType, UnitAbbreviationsCache.FallbackCulture);
}

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());
throw new AmbiguousUnitParseException(
$"Cannot parse \"{unitAbbreviation}\" since it could be either of these: {unitsCsv}");
(Enum Unit, string Abbreviation)[] matches = FindMatchingUnits(unitAbbreviation, enumValues, formatProvider);
switch(matches.Length)
{
case 1:
return matches[0].Unit;
case 0:
// Retry with fallback culture, if different.
if (Equals(formatProvider, UnitAbbreviationsCache.FallbackCulture))
{
throw new UnitNotFoundException($"Unit not found with abbreviation [{unitAbbreviation}] for unit type [{unitType}].");
}

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}");
}
}
}

Expand Down Expand Up @@ -138,7 +130,8 @@ internal static string NormalizeUnitString(string unitAbbreviation)
/// <param name="unit">The unit enum value as out result.</param>
/// <typeparam name="TUnitType">Type of unit enum.</typeparam>
/// <returns>True if successful.</returns>
public bool TryParse<TUnitType>([NotNullWhen(true)]string? unitAbbreviation, out TUnitType unit) where TUnitType : struct, Enum
public bool TryParse<TUnitType>([NotNullWhen(true)]string? unitAbbreviation, out TUnitType unit)
where TUnitType : struct, Enum
{
return TryParse(unitAbbreviation, null, out unit);
}
Expand All @@ -151,7 +144,8 @@ public bool TryParse<TUnitType>([NotNullWhen(true)]string? unitAbbreviation, out
/// <param name="unit">The unit enum value as out result.</param>
/// <typeparam name="TUnitType">Type of unit enum.</typeparam>
/// <returns>True if successful.</returns>
public bool TryParse<TUnitType>([NotNullWhen(true)]string? unitAbbreviation, IFormatProvider? formatProvider, out TUnitType unit) where TUnitType : struct, Enum
public bool TryParse<TUnitType>([NotNullWhen(true)]string? unitAbbreviation, IFormatProvider? formatProvider, out TUnitType unit)
where TUnitType : struct, Enum
{
unit = default;

Expand Down Expand Up @@ -184,34 +178,58 @@ public bool TryParse([NotNullWhen(true)] string? unitAbbreviation, Type unitType
/// <returns>True if successful.</returns>
public bool TryParse([NotNullWhen(true)] string? unitAbbreviation, Type unitType, IFormatProvider? formatProvider, [NotNullWhen(true)] out Enum? unit)
{
unit = null;
if (unitAbbreviation == null)
{
unit = default;
return false;
}

unitAbbreviation = unitAbbreviation.Trim();
unit = default;
Enum[] enumValues = Enum.GetValues(unitType).Cast<Enum>().ToArray();
(Enum Unit, string Abbreviation)[] matches = FindMatchingUnits(unitAbbreviation, enumValues, formatProvider);

var enumValues = Enum.GetValues(unitType).Cast<Enum>();
var stringUnitPairs = _unitAbbreviationsCache.GetStringUnitPairs(enumValues, formatProvider);
var matches = stringUnitPairs.Where(pair => pair.Item1.Equals(unitAbbreviation, StringComparison.OrdinalIgnoreCase)).ToArray();
if (matches.Length == 1)
{
unit = matches[0].Unit;
return true;
}

if (matches.Length != 0 || Equals(formatProvider, UnitAbbreviationsCache.FallbackCulture))
{
return false; // either there are duplicates or nothing was matched and we're already using the fallback culture
}

if(matches.Length == 0)
// retry the lookup using the fallback culture
matches = FindMatchingUnits(unitAbbreviation, enumValues, UnitAbbreviationsCache.FallbackCulture);
if (matches.Length != 1)
{
return false;
}

unit = matches[0].Unit;
return true;
}

private (Enum Unit, string Abbreviation)[] FindMatchingUnits(string unitAbbreviation, IEnumerable<Enum> enumValues, IFormatProvider? formatProvider)
{
// 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();

if (matches.Length == 0)
{
unitAbbreviation = NormalizeUnitString(unitAbbreviation);
matches = stringUnitPairs.Where(pair => pair.Item1.Equals(unitAbbreviation, StringComparison.OrdinalIgnoreCase)).ToArray();
matches = stringUnitPairs.Where(pair => pair.Item2.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)
matches = stringUnitPairs.Where(pair => pair.Item1.Equals(unitAbbreviation)).ToArray();

if(matches.Length != 1)
return false;
if (matches.Length > 1)
{
matches = stringUnitPairs.Where(pair => pair.Item2.Equals(unitAbbreviation)).ToArray();
}

unit = (Enum)Enum.ToObject(unitType, matches[ 0 ].Unit);
return true;
return matches;
}
}
}