Skip to content
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

Fix overridden abbrev #833

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Fix overridden abbrev #833

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
8b01d68
Add failing tests
tmilnthorp Sep 8, 2020
a625c7f
Wrap GetUnitsForAbbreviation in UnitAbbreviationsCache to allow for f…
tmilnthorp Sep 8, 2020
bc5f8c4
Fix whitespace for code consistency
tmilnthorp Sep 8, 2020
5be4cf7
Don't modify default UnitAbbreviationsCache in test
tmilnthorp Sep 9, 2020
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
19 changes: 19 additions & 0 deletions UnitsNet.Tests/UnitAbbreviationsCacheTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -296,6 +296,25 @@ public void MapUnitToDefaultAbbreviation_GivenCustomAbbreviation_SetsAbbreviatio
Assert.Equal("1 m^2", Area.FromSquareMeters(1).ToString(newZealandCulture));
}

[Fact]
public void MapUnitOverrideTest()
{
UnitAbbreviationsCache.Default.MapUnitToDefaultAbbreviation(MassFlowUnit.GramPerSecond, NorwegianCulture, "grams/second");
tmilnthorp marked this conversation as resolved.
Show resolved Hide resolved
Assert.Equal(MassFlow.FromPoundsPerSecond(10), Quantity.Parse(NorwegianCulture, typeof(MassFlow), "10 lb/s"));
}

[Fact]
public void MapUnitToDefaultAbbreviation_AddsNewUnitAsAdditionalOverride()
{
var cache = new UnitAbbreviationsCache();
var abbreviationsBefore = cache.GetUnitAbbreviations(MassFlowUnit.GramPerSecond, NorwegianCulture);

cache.MapUnitToDefaultAbbreviation(MassFlowUnit.GramPerSecond, NorwegianCulture, "grams/second");

var abbreviationsAfter = cache.GetUnitAbbreviations(MassFlowUnit.GramPerSecond, NorwegianCulture);
Assert.Equal(abbreviationsBefore.Length + 1, abbreviationsAfter.Length);
}

/// <summary>
/// Convenience method to the proper culture parameter type.
/// </summary>
Expand Down
34 changes: 29 additions & 5 deletions UnitsNet/CustomCode/UnitAbbreviationsCache.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -276,9 +276,11 @@ public string[] GetUnitAbbreviations(Type unitType, int unitValue, IFormatProvid
if(!TryGetUnitValueAbbreviationLookup(unitType, formatProvider, out var lookup))
return formatProvider != FallbackCulture ? GetUnitAbbreviations(unitType, unitValue, FallbackCulture) : new string[] { };

var abbreviations = lookup!.GetAbbreviationsForUnit(unitValue);
if(abbreviations.Count == 0)
return formatProvider != FallbackCulture ? GetUnitAbbreviations(unitType, unitValue, FallbackCulture) : new string[] { };
var abbreviations = new HashSet<string>();
abbreviations.UnionWith(lookup!.GetAbbreviationsForUnit(unitValue));

if(formatProvider != FallbackCulture)
abbreviations.UnionWith(GetUnitAbbreviations(unitType, unitValue, FallbackCulture));
Copy link
Owner

Choose a reason for hiding this comment

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

At first glance, I'm not sure about this union.

If you call GetUnitAbbreviations(typeof(LengthUnit), 1, russianCulture) then from this public method signature, isn't it unexpected to also receive English abbreviations?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

A good question :) Should it? Currently you can get English abbreviations from GetUnitAbbreviations, but only if no Russian ones exist.

The parser obviously will not fall back without it. It's either that, or the parser logic will need to include the fallback logic. I wanted to keep everything contained.

Copy link
Owner

Choose a reason for hiding this comment

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

I'm really not sure. How about adding a new parameter bool includeFallbackCulture = false and call it with true from our own usages where we need to?

Copy link
Owner

Choose a reason for hiding this comment

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

I still think we should not do a union here indiscriminately. What do you think of my proposal of a new argument?


return abbreviations.ToArray();
}
Expand All @@ -297,10 +299,32 @@ public string[] GetAllUnitAbbreviationsForQuantity(Type unitEnumType, IFormatPro
if(!TryGetUnitValueAbbreviationLookup(unitEnumType, formatProvider, out var lookup))
return formatProvider != FallbackCulture ? GetAllUnitAbbreviationsForQuantity(unitEnumType, FallbackCulture) : new string[] { };

return lookup!.GetAllUnitAbbreviationsForQuantity();
var allAbbreviations = new HashSet<string>();
allAbbreviations.UnionWith(lookup!.GetAllUnitAbbreviationsForQuantity());

if(formatProvider != FallbackCulture)
allAbbreviations.UnionWith(GetAllUnitAbbreviationsForQuantity(unitEnumType, FallbackCulture));

return allAbbreviations.ToArray();
}

internal List<int> GetUnitsForAbbreviation(Type unitType, IFormatProvider? formatProvider, string abbreviation, bool ignoreCase)
{
formatProvider = formatProvider ?? CultureInfo.CurrentUICulture;

if(!TryGetUnitValueAbbreviationLookup(unitType, formatProvider, out var lookup))
return formatProvider != FallbackCulture ? GetUnitsForAbbreviation(unitType, FallbackCulture, abbreviation, ignoreCase) : new List<int>();

var units = new HashSet<int>();
units.UnionWith(lookup!.GetUnitsForAbbreviation(abbreviation, ignoreCase));

if(formatProvider != FallbackCulture)
units.UnionWith(GetUnitsForAbbreviation(unitType, FallbackCulture, abbreviation, ignoreCase));

return units.ToList();
}

internal bool TryGetUnitValueAbbreviationLookup(Type unitType, IFormatProvider? formatProvider, out UnitValueAbbreviationLookup? unitToAbbreviations)
private bool TryGetUnitValueAbbreviationLookup(Type unitType, IFormatProvider? formatProvider, out UnitValueAbbreviationLookup? unitToAbbreviations)
{
unitToAbbreviations = null;

Expand Down
18 changes: 6 additions & 12 deletions UnitsNet/CustomCode/UnitParser.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,20 +72,17 @@ public Enum Parse([NotNull] string unitAbbreviation, Type unitType, IFormatProvi
if (unitAbbreviation == null) throw new ArgumentNullException(nameof(unitAbbreviation));
unitAbbreviation = unitAbbreviation.Trim();

if(!_unitAbbreviationsCache.TryGetUnitValueAbbreviationLookup(unitType, formatProvider, out var abbreviations))
throw new UnitNotFoundException($"No abbreviations defined for unit type [{unitType}] for culture [{formatProvider}].");

var unitIntValues = abbreviations!.GetUnitsForAbbreviation(unitAbbreviation, ignoreCase: true);
var unitIntValues = _unitAbbreviationsCache.GetUnitsForAbbreviation(unitType, formatProvider, unitAbbreviation, ignoreCase: true);

if (unitIntValues.Count == 0)
{
unitAbbreviation = NormalizeUnitString(unitAbbreviation);
unitIntValues = abbreviations.GetUnitsForAbbreviation(unitAbbreviation, ignoreCase: true);
unitIntValues = _unitAbbreviationsCache.GetUnitsForAbbreviation(unitType, formatProvider, unitAbbreviation, ignoreCase: true);
}

// Narrow the search if too many hits, for example Megabar "Mbar" and Millibar "mbar" need to be distinguished
if (unitIntValues.Count > 1)
unitIntValues = abbreviations.GetUnitsForAbbreviation(unitAbbreviation, ignoreCase: false);
unitIntValues = _unitAbbreviationsCache.GetUnitsForAbbreviation(unitType, formatProvider, unitAbbreviation, ignoreCase: false);

switch (unitIntValues.Count)
{
Expand Down Expand Up @@ -197,20 +194,17 @@ public bool TryParse(string? unitAbbreviation, Type unitType, IFormatProvider? f
unitAbbreviation = unitAbbreviation.Trim();
unit = default;

if(!_unitAbbreviationsCache.TryGetUnitValueAbbreviationLookup(unitType, formatProvider, out var abbreviations))
return false;

var unitIntValues = abbreviations!.GetUnitsForAbbreviation(unitAbbreviation, ignoreCase: true);
var unitIntValues = _unitAbbreviationsCache.GetUnitsForAbbreviation(unitType, formatProvider, unitAbbreviation, ignoreCase: true);

if (unitIntValues.Count == 0)
{
unitAbbreviation = NormalizeUnitString(unitAbbreviation);
unitIntValues = abbreviations.GetUnitsForAbbreviation(unitAbbreviation, ignoreCase: true);
unitIntValues = _unitAbbreviationsCache.GetUnitsForAbbreviation(unitType, formatProvider, unitAbbreviation, ignoreCase: true);
}

// Narrow the search if too many hits, for example Megabar "Mbar" and Millibar "mbar" need to be distinguished
if (unitIntValues.Count > 1)
unitIntValues = abbreviations.GetUnitsForAbbreviation(unitAbbreviation, ignoreCase: false);
unitIntValues = _unitAbbreviationsCache.GetUnitsForAbbreviation(unitType, formatProvider, unitAbbreviation, ignoreCase: false);

if(unitIntValues.Count != 1)
return false;
Expand Down
1 change: 0 additions & 1 deletion UnitsNet/CustomCode/UnitValueAbbreviationLookup.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,6 @@ internal void Add(int unit, string abbreviation, bool setAsDefault = false)
abbreviationsForUnit.Insert(0, abbreviation);
else
abbreviationsForUnit.Add(abbreviation);

}
}
}