No fall-back culture when using UnitAbbreviationsCache.CreateEmpty

[lipchev]

Have a look at this test (it's targeting the v6 but that should be reproducible in v5)

        [Fact]
        public void MapUnitToDefaultAbbreviation_GivenUnitAndNoCulture_SetsDefaultAbbreviationForUnitForCurrentCulture()
        {
            using var cultureScope = new CultureScope(NorwegianCultureName);
            var cache = new UnitAbbreviationsCache();

            cache.MapUnitToDefaultAbbreviation(MassUnit.Gram, "zz");

            Assert.Equal("zz", cache.GetDefaultAbbreviation(MassUnit.Gram));
            Assert.Equal("g", cache.GetDefaultAbbreviation(MassUnit.Gram, AmericanCulture));
        }

It fails with

Expected: "g"
Actual: ""

Everything works fine when initializing with the list of QuantityInfo:

        [Fact]
        public void MapUnitToDefaultAbbreviation_WithDefaultLookup_GivenUnitAndNoCulture_SetsDefaultAbbreviationForUnitForCurrentCulture()
        {
            using var cultureScope = new CultureScope(NorwegianCultureName);
            var cache = UnitAbbreviationsCache.CreateDefault();

            cache.MapUnitToDefaultAbbreviation(MassUnit.Gram, "zz");

            Assert.Equal("zz", cache.GetDefaultAbbreviation(MassUnit.Gram));
            Assert.Equal("g", cache.GetDefaultAbbreviation(MassUnit.Gram, AmericanCulture));
        }

The reason why this doesn't work is not immediately obvious- it's because when calling MapUnitToDefaultAbbreviation, when the UnitInfo is not found in the QuantityInfoLookup the PerformAbbreviationMapping creates a dummy UnitInfo which doesn't have anything set for the QuantityName property.

In my opinion, the best solution would be to use these constructors:

        /// <summary>
        ///      Create an instance of the cache and load all the built-in quantities defined in the library.
        /// </summary>
        /// <returns>Instance for mapping any of the built-in units.</returns>
        public UnitAbbreviationsCache()
            :this(UnitsNetSetup.Default.QuantityInfoLookup)
        {
        }
        
        /// <summary>
        ///     Creates an instance of the cache using the specified set of quantities.
        /// </summary>
        /// <returns>Instance for mapping the units of the provided quantities.</returns>
        public UnitAbbreviationsCache(IReadOnlyCollection<QuantityInfo> quantities)
            :this(new QuantityInfoLookup(quantities))
        {
        }

        /// <summary>
        ///     Creates an instance of the cache using the specified set of quantities.
        /// </summary>
        /// <remarks>
        ///     Access type is <c>internal</c> until this class is matured and ready for external use.
        /// </remarks>
        internal UnitAbbreviationsCache(QuantityInfoLookup quantities)
        {
            Quantities = quantities;
        }

and throw an exception when the UnitInfo isn't found in the cache (when mapping or getting).

[angularsen]

If you meant that

Assert.Equal("g", cache.GetDefaultAbbreviation(MassUnit.Gram, AmericanCulture));

should throw instead of returning empty string, then I agree 👌

[angularsen]

I believe this is kind of a v6 issue, where ctor creates an empty unit abbreviations cache, while in v5 the ctor loads the default cache.

[lipchev]

I believe this is kind of a v6 issue, where ctor creates an empty unit abbreviations cache, while in v5 the ctor loads the default cache.

For v5 this would be reproducible when using CreateEmpty(), but I doubt many are using it that way..

[lipchev]

If you meant that

Assert.Equal("g", cache.GetDefaultAbbreviation(MassUnit.Gram, AmericanCulture));

should throw instead of returning empty string, then I agree 👌

@angularsen Ok, after #1507 is merged, I'm want to make the following change to the GetDefaultAbbreviation method:

        /// <inheritdoc cref="GetDefaultAbbreviation{TUnitType}" />
        /// <param name="unitKey">The key representing the unit type and value.</param>
        /// <param name="culture">The localization culture. Defaults to <see cref="CultureInfo.CurrentCulture" /> if null.</param>
        /// <exception cref="UnitNotFoundException">
        ///     Thrown when no unit information is found for the specified
        ///     <paramref name="unitKey" />.
        /// </exception>
        /// <exception cref="InvalidOperationException">
        ///     Thrown when no abbreviations are mapped for the specified unit.
        /// </exception>
        public string GetDefaultAbbreviation(UnitKey unitKey, CultureInfo? culture = null)
        {
            IReadOnlyList<string> abbreviations = GetUnitAbbreviations(unitKey, culture);
            if (abbreviations.Count == 0)
            {
                throw new InvalidOperationException($"No abbreviations were found for {unitKey.UnitType.Name}.{(Enum)unitKey}. Make sure that the unit abbreviations are mapped.");
            }

            return abbreviations[0];
        }

Normally, this should only throw the UnitNotFoundException when called with invalid unit type. The InvalidOperationException should be thrown when the unit is found, but there are no abbreviations mapped for it. This means that, unlike before, "" should be mapped explicitly (like the DecimalFraction). This is already the case for all existing units, with the exception of the culinary volumes:

Assert.All() Failure: 7 out of 54 items in the collection did not pass.
[1]: Item: AuTablespoon
Error: No abbreviations were found for VolumeUnit.AuTablespoon. Make sure that the unit abbreviations are mapped.
[38]: Item: MetricCup
Error: No abbreviations were found for VolumeUnit.MetricCup. Make sure that the unit abbreviations are mapped.
[44]: Item: UkTablespoon
Error: No abbreviations were found for VolumeUnit.UkTablespoon. Make sure that the unit abbreviations are mapped.
[46]: Item: UsCustomaryCup
Error: No abbreviations were found for VolumeUnit.UsCustomaryCup. Make sure that the unit abbreviations are mapped.
[48]: Item: UsLegalCup
Error: No abbreviations were found for VolumeUnit.UsLegalCup. Make sure that the unit abbreviations are mapped.
[52]: Item: UsTablespoon
Error: No abbreviations were found for VolumeUnit.UsTablespoon. Make sure that the unit abbreviations are mapped.
[53]: Item: UsTeaspoon
Error: No abbreviations were found for VolumeUnit.UsTeaspoon. Make sure that the unit abbreviations are mapped.

Initially I was going to just add "Abbreviations": [ "" ] to each of them, but now I don't think that's a good idea. I think it would be better to just give them a common name (without localization 😄 ). This is already the case for "Abbreviations": [ "metric cup" ]. Here's what I'm thinking of:

• "Abbreviations": [ "tablespoon (U.S.)" ]
• "Abbreviations": [ "tablespoon (A.U.)" ]
• "Abbreviations": [ "tablespoon (U.K.)" ]
• "Abbreviations": [ "teaspoon (U.S.)" ]
• "Abbreviations": ["cup (U.S. customary)"]
• "Abbreviations": ["cup (U.S.)"]

[angularsen]

Yeah, that sounds reasonable.

• Explicitly map "" for DecimalFraction and friends
• Common names for units where empty string is not representative

[github-actions]

This issue is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 7 days.

[lipchev]

@angularsen Another thing I'd like to confirm is regarding the result of this function:

UnitsNet/UnitsNet/CustomCode/UnitAbbreviationsCache.cs

Lines 262 to 267 in 80ce17c

	public IReadOnlyList<string> GetAllUnitAbbreviationsForQuantity(Type unitEnumType, IFormatProvider? formatProvider = null)
	{
	var enumValues = Enum.GetValues(unitEnumType).Cast<Enum>();
	var all = GetStringUnitPairs(enumValues, formatProvider);
	return all.Select(pair => pair.Item2).ToList();
	}

In what situations do you think we should throw:

1. Given a type that's not an Enum (ArgumentException)
2. Given a type that's not part of the units registered in the QuantityInfoLookup (UnitNotFoundException)
3. Never (return [])

[lipchev]

And secondly, there is also the matter of the FromUnitAbbreviation overloads that operate on the full set of quantities:

UnitsNet/UnitsNet/CustomCode/Quantity.cs

Lines 90 to 122 in 80ce17c

	/// <summary>
	/// Dynamically construct a quantity from a numeric value and a unit abbreviation.
	/// </summary>
	/// <remarks>
	/// This method is currently not optimized for performance and will enumerate all units and their unit abbreviations each time.<br/>
	/// Unit abbreviation matching is case-insensitive.<br/>
	/// <br/>
	/// This will fail if more than one unit across all quantities share the same unit abbreviation.<br/>
	/// Prefer <see cref="From(double,System.Enum)"/> or <see cref="From(double,string,string)"/> instead.
	/// </remarks>
	/// <param name="formatProvider">The format provider to use for lookup. Defaults to <see cref="CultureInfo.CurrentCulture" /> if null.</param>
	/// <param name="value">Numeric value.</param>
	/// <param name="unitAbbreviation">Unit abbreviation, such as "kg" for <see cref="MassUnit.Kilogram"/>.</param>
	/// <returns>An <see cref="IQuantity"/> object.</returns>
	/// <exception cref="UnitNotFoundException">Unit abbreviation is not known.</exception>
	/// <exception cref="AmbiguousUnitParseException">Multiple units found matching the given unit abbreviation.</exception>
	public static IQuantity FromUnitAbbreviation(IFormatProvider? formatProvider, double value, string unitAbbreviation)
	{
	// TODO Optimize this with UnitValueAbbreviationLookup via UnitAbbreviationsCache.TryGetUnitValueAbbreviationLookup.
	List<Enum> units = GetUnitsForAbbreviation(formatProvider, unitAbbreviation);
	if (units.Count > 1)
	{
	throw new AmbiguousUnitParseException($"Multiple units found matching the given unit abbreviation: {unitAbbreviation}");
	}
	if (units.Count == 0)
	{
	throw new UnitNotFoundException($"Unit abbreviation {unitAbbreviation} is not known. Did you pass in a custom unit abbreviation defined outside the UnitsNet library? This is currently not supported.");
	}
	Enum unit = units.Single();
	return From(value, unit);
	}

I'm aware of the ambiguity when parsing cm and all of that, but still I think that we should make an implementation that mimics the behavior of the regular Parse function. Here's what I got for it in the UnitParser:

    internal UnitInfo GetUnitFromAbbreviation(string unitAbbreviation, CultureInfo? culture)  // TODO make public?
    {
        if (unitAbbreviation == null) throw new ArgumentNullException(nameof(unitAbbreviation));
        culture ??= CultureInfo.CurrentCulture;
        unitAbbreviation = unitAbbreviation.Trim();
        StringComparison comparison = StringComparison.Ordinal;
        while (true)
        {
            List<(UnitInfo UnitInfo, string Abbreviation)> matches = FindAllMatchingUnitsForCulture(unitAbbreviation, culture, comparison);
            switch (matches.Count)
            {
                case 1:
                    return matches[0].UnitInfo;
                case 0:
                    // Retry with fallback culture, if different.
                    if (UnitAbbreviationsCache.HasFallbackCulture(culture))
                    {
                        culture = UnitAbbreviationsCache.FallbackCulture;
                        continue;
                    }
                    
                    var normalizedUnitString = NormalizeUnitString(unitAbbreviation);
                    if (normalizedUnitString != unitAbbreviation)
                    {
                        unitAbbreviation = normalizedUnitString;
                        continue;
                    }

                    if (comparison == StringComparison.Ordinal)
                    {
                        comparison = StringComparison.OrdinalIgnoreCase;
                        continue;
                    }

                    throw new UnitNotFoundException($"Unit not found with abbreviation [{unitAbbreviation}].");
                default:
                    var unitsCsv = string.Join(", ", matches.Select(x => $"{x.UnitInfo.Name} (\"{x.Abbreviation}\")").OrderBy(x => x));
                    throw new AmbiguousUnitParseException($"Cannot parse \"{unitAbbreviation}\" since it matches multiple units: {unitsCsv}.");
            }
        }
    }

This would change two of the tests in QuantityTests.cs from Throws to Returns:

        [Fact]
        public void FromUnitAbbreviation_MatchingFallbackCulture_ReturnsQuantity()
        {
            // The localized abbreviation is "см"
            IQuantity quantity = Quantity.FromUnitAbbreviation(Russian, 5, "cm");  
            Assert.Equal(5, quantity.Value);
            Assert.Equal(LengthUnit.Centimeter, quantity.Unit);
        }

[github-actions]

This issue is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 7 days.

[angularsen]

In what situations do you think we should throw:

1. Given a type that's not an Enum (ArgumentException)
2. Given a type that's not part of the units registered in the QuantityInfoLookup (UnitNotFoundException)
3. Never (return [])

Throw for 1 and 2, it's clearly a mistake by the consumer and we should help them.. catch it, pun intended.

[angularsen]

but still I think that we should make an implementation that mimics the behavior of the regular Parse function

Agreed, it should follow the exact same case-insensitive-then-case-sensitive logic we have for parsing to be consistent.