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.

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

Overrides of unit abbreviations in non-default cultures causes Quantity.Parse to fail #832

Closed
hemenwaynick opened this issue Sep 4, 2020 · 6 comments · May be fixed by #833
Closed

Overrides of unit abbreviations in non-default cultures causes Quantity.Parse to fail #832

hemenwaynick opened this issue Sep 4, 2020 · 6 comments · May be fixed by #833

Comments

@hemenwaynick
Copy link

Describe the bug
UnitsNet offers a public method that allows consumers of the NuGet package to override the default abbreviations used for different units.

For example, one can write

UnitAbbreviationsCache.Default.MapUnitToDefaultAbbreviation(typeof(MassFlowUnit), (int)MassFlowUnit.GramPerSecond, CultureInfo.CurrentCulture, "grams/second")

in order to use "grams/second" instead of "g/s" as the displayed abbreviation.

However, if the current culture is something other than en-US, some problems arise when attempting to call

UnitsNet.Quantity.Parse(Type quantityType, string quantityString)

after performing the above override.

If we had an override as in the example above, attempting to parse any mass flow string that doesn't use the "grams/second" abbreviation would fail in a non-English culture.

For example,

Quantity.Parse(typeof(MassFlow), "10 lb/s")

would fail if the current culture were de-DE.

To Reproduce
(1) Add an override similar to the one in the description, using a culture other than en-US.
(2) Call Quantity.Parse in the culture that was used specified in your override, and make this call on a quantity string that uses a unit abbreviation other than the one you specified in the override.
The parse should fail.
(Bug was found in UnitsNet 4.65.0 but hasn't been fixed to my knowledge.)

Expected behavior
Calls to Quantity.Parse that succeed without overrides to unit abbreviations should behave the same way with overrides.
Without overrides, UnitsNet falls back to using en-US for non-English cultures (in most cases). The override is causing UnitsNet to ignore the fallback abbreviations.

Screenshots
The problem appears to lie in how the following method in UnitAbbreviationsCache.cs is handled. A return value of true is given even when the lookup that ends up getting stored in unitToAbbreviations may not include any abbreviations other than ones that have been specified in an override. At other points in this file, a true return value from TryGetUnitValueAbbreviationLookup will cause UnitsNet to use the incomplete lookup and ignore the fallback culture.
image

Additional context
I encountered this bug when overriding unit abbreviations, but it would seem to apply also to cases where one simply adds new abbreviations as well.

@angularsen
Copy link
Owner

Excellent description, thank you!

Would you be willing to attempt a pull request that adds a unit test that reproduces the bug and a bug fix that passes the test?

I might find some time to dig into this later, but it seems like you have pretty much nailed the problem already.

@hemenwaynick
Copy link
Author

hemenwaynick commented Sep 5, 2020

I'll see if I can find some time this weekend. Will let you know.

@tmilnthorp
Copy link
Collaborator

I believe the issue is the quantity parser calling into TryGetUnitValueAbbreviationLookup directly. This should be private.

The UnitAbbreviationsCache wraps up all fallback behavior, and should similarly wrap up a GetUnitsForAbbreviation method to include all fallbacks.

@hemenwaynick
Copy link
Author

@tmilnthorp Sounds good.

I can create a PR at some point for this one (hopefully next weekend), but if anyone wants to put in a fix sooner, I'm cool with that too :)

We have a workaround in Discovery, so this isn't a high priority at the moment.

@tmilnthorp
Copy link
Collaborator

I've added PR #833

@stale
Copy link

stale bot commented Nov 7, 2020

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the wontfix label Nov 7, 2020
@stale stale bot closed this as completed Nov 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants