UnitAbbreviationsCache.CreateEmpty should use the default QuantityInfoLookup

[lipchev]

• UnitAbbreviationsCache: refactored the constructors / Create factory methods, with the Map/Get methods throwing an exception for missing units / unmapped abbreviations
• UnitAbbreviationsCache: replaced the string[] return types with IReadOnlyList (breaking change for .Length which now becomes .Count)
• UnitAbbreviationsCache: removed the overload accepting a UnitInfo
• UnitParser: optimized the unit-parsing-with-fallback-culture methods, making the FromUnitAbbreviation behavior consistent with that of ParseUnit
• UnitParser: UnitInfo GetUnitFromAbbreviation(string unitAbbreviation, IFormatProvider? formatProvider) : flipped the parameters and made the method public (this is optional)
• optimized the Mass.ParseUnit calls (avoiding the UnitInfo lookups)
• completed the coverage of the UnitParser and UnitAbbreviationsCache

fixes #1509

[lipchev]

@angularsen There are still some tests missing but I'm on my way out- will try to add them when I get back. Please have a look at the remaining TODOs and tell me if you agree with my comments.

[angularsen]

I think this looks very good, just a few suggestions. Just merge when ready.

[angularsen]

I'm not sure I understand what this is testing? cache is never used, so we test both constructing a new local UnitAbbreviationsCache instance + a lookup of the global default UnitAbbreviationsCache?

[lipchev]

Originally I was using this overload for testing the initialization of the UnitAbbreviationsCache, as it avoids the call to the QuantityInfoLookup but now that the Mass.GetAbbreviation call does it instead (using the internal overload)- I could switch it over and hopefully remove the method (see the TODO - we shouldn't accept a UnitInfo as an argument, as it might not be the same UnitInfo that was registered earlier).

[lipchev]

the overload (and it's one test) have been removed in f9a2766

[lipchev]

cache is never used

Oh I see, this isn't doing anything anymore.. I'll just remove them.

[angularsen]

I kind of liked how the previous default ctor created an empty instance, and in order to create a new instance with defaults loaded you'd have to call CreateDefault(). Most people will use the singleton instance anyway, but it just seems more conventional to me.

[lipchev]

The goal here is to have the list of QuantityInfos provided via the constructor, unlike before where a new UnitInfo was created when trying to get / map an abbreviation to a TUnit that doesn't exist in the QuantityInfoLookup.

The natural meaning IMO is that this creates an empty cache (meaning nothing is cached) using the default quantities (for mapping). In the 🐲 version I've got a few more constructor overloads, which should make it easier to construct / customize the default list.

[angularsen]

Okay maybe I just don't understand it, to me it seems like we're getting a cache with all built-in unit abbreviations when calling new UnitAbbreviationsCache() ?

[lipchev]

Yes, but it's an empty cache, as one would expect from a call to new Cache()

[lipchev]

Of course it has always been an empty cache, it's just that before it was an empty cache and an empty list of quantities to map (which was problematic).

[angularsen]

Right, and this is more a theoretical exercise to challenge the design, but it seems nice to be able to use the pieces on their own without involving all the built-in unit definitions.

// For some reason, we don't want to initialize with HowMuch at this point
var cache = new UnitAbbreviationsCache();

// Instead add it later, in one of two ways
cache.AddQuantity(HowMuch.Info);
cache.Quantities.Add(HowMuch.Info);

// Then map the abbreviations
cache.MapUnitToDefaultAbbreviation(HowMuchUnit.Some, "sum");

[angularsen]

I guess it comes down to that it feels more intuitive to get this behavior from the default ctor, than having to call CreateEmpty().

[lipchev]

Oh, that would be very difficult to accomplish.. Unless you want to add a message bus or something 😄. Of course we could have an assumption that the user would call AddQuantity before attempting to Map or Get it but again- that would make it impossible to use the FrozenDictionary, not to mention the concurrency issues once again, please let's not.. (insert meme here).

The UnitParser is now really lightweight, the user should be able to create one on the fly, but for the initial mapping (of the DefaultAbrreviations) I really hope that we could use something like UnitsNet.Configure(c=> ... add custom stuff here) at the app start.

[angularsen]

Alright, this is not an important point for me. I get that the new design forces you to either use the default ctor, with built-in quantities, or pass your own quantities, and that it becomes immutable at that point. That's fine.

We'll still need to accomodate adding custom quantities at runtime for the default singleton though, but this should be doable either by calling something early on startup as you say or swapping the immutable instance with a new one.

[angularsen]

Nit, !Equals(culture, CultureInfo.InvariantCulture) is more explicit.

The culture.Name != FallbackCulture.Name must stay though, since the name is what matters and not the culture info configuration.

[angularsen]

One thing I don't quite get, FallbackCulture is InvariantCulture. So doesn't this really just check twice that the given culture is not invariant culture?

[angularsen]

I see there's some quirks regarding this here:

        private static AbbreviationMapKey GetAbbreviationMapKey(UnitInfo unitInfo, CultureInfo culture)
        {
            // In order to support running in "invariant mode" (DOTNET_SYSTEM_GLOBALIZATION_INVARIANT=1) the FallbackCulture is set to the InvariantCulture.
            // However, if we want to avoid having two entries in the cache ("", "en-US"), we need to map the invariant culture name to the primary localization language.
            return new AbbreviationMapKey(unitInfo.UnitKey, culture.Name == string.Empty ? "en-US" : culture.Name);
        }

[angularsen]

Suggestion:

private const string DefaultCultureName = "en-US";

internal static bool HasFallbackCulture(CultureInfo culture)
{
    return !culture.Equals(CultureInfo.InvariantCulture) && culture.Name != DefaultCultureName;
}

And reuse DefaultCultureName in GetAbbreviationMapKey.

It still doesn't feel clean, there's something weird about this whole FallbackCulture being InvariantCulture, and then mapped to en-US, but I don't have a better suggestion right now.

[lipchev]

Sorry, forgot about this one: culture.Equals is a bit more expensive than simply checking the name:

  public override bool Equals([NotNullWhen(true)] object? value)
  {
    if (this == value)
      return true;
    return value is CultureInfo cultureInfo && this.Name.Equals(cultureInfo.Name) && this.CompareInfo.Equals((object) cultureInfo.CompareInfo);
  }

and I think there is a potential risk for key-duplication if the user uses a custom culture (derived from CultureInfo.InvariantCulture).

As for the constant- I tried it, but it looked weird without the accompanying comment (which got even more vague when the string value got hidden by the constant name 😄 ).

PS This looks weird to me too, but I don't see how to improve it..

[lipchev]

This is the unfortunate side effect of moving away from the arrays, but there is no way around it..

I'm merging it..