Fix overridden abbrev #833
Conversation
…allback/override behavior
Codecov Report
@@ Coverage Diff @@
## master #833 +/- ##
=======================================
Coverage 80.54% 80.55%
=======================================
Files 281 281
Lines 41925 41936 +11
=======================================
+ Hits 33770 33783 +13
+ Misses 8155 8153 -2
Continue to review full report at Codecov.
|
| abbreviations.UnionWith(lookup!.GetAbbreviationsForUnit(unitValue)); | ||
|
|
||
| if(formatProvider != FallbackCulture) | ||
| abbreviations.UnionWith(GetUnitAbbreviations(unitType, unitValue, FallbackCulture)); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
I still think we should not do a union here indiscriminately. What do you think of my proposal of a new argument?
|
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. |
|
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. |
angularsen
added
pinned
Fixes #832