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