Move unit abbreviation strings into culture-specific resource files #1210
Conversation
|
Check #1126 after merge |
UnitsNet.Tests/GeneratedCode/TestsBase/AccelerationTestsBase.g.cs
Outdated
Show resolved
Hide resolved
|
@angularsen curious if you like the direction this is headed. I need to re-implement |
| /// <summary> | ||
| /// The per-culture abbreviations. To add a custom default abbreviation, add to the beginning of the list. | ||
| /// </summary> | ||
| public static IDictionary<(CultureInfo Culture, {_unitEnumName} Unit), List<string>> Abbreviations {{ get; }} |
There was a problem hiding this comment.
Before even reading the rest, shouldn't this rather sit in QuantityInfo?
- This is where units and other data for a quantity is stored
- Reusable/extensible when working with
IQuantity - Reduce member count per quantity (binary size)
There was a problem hiding this comment.
Reading more, yes, GetAbbreviations() etc seems to me would sit nicely in QuantityInfo.
There was a problem hiding this comment.
@angularsen I like that. Let me give it a shot here
| catch | ||
| { | ||
| throw; | ||
| } |
There was a problem hiding this comment.
This doesn't seem too useful, maybe just omit the catch in this case?
| abbreviations = new List<string>(); | ||
|
|
||
| const string resourceName = $""UnitsNet.GeneratedCode.Resources.{_quantity.Name}""; | ||
| var resourceManager = new ResourceManager(resourceName, typeof({_quantity.Name}).Assembly); |
There was a problem hiding this comment.
I'm not familiar with manually instantiating ResourceManager. Should we cache and reuse these instances?
There was a problem hiding this comment.
Generally though, I think you are on to something good about reducing the generated code and moving things like localization to files that are read at runtime.
The JIT clearly has issues with our codebase. Not sure if it is the large number of types/members or if it is just the large amount of code, or what exactly.
There was a problem hiding this comment.
Moving things into QuantityInfo is another way to avoid duplicating methods N times for N quantities.
The biggest hit is M members for M units though, which is roughly 10x the number of quantities.
There was a problem hiding this comment.
@angularsen I am doing it lazily to load into a List. However there is some caching done internally, if my read of the docs is correct.
|
@angularsen moved to I have no clue why auto-gen for |
|
@angularsen now at the point where everything works - however routing everything via |
FYI tests run locally on re-run since the scope is narrower. Not sure what to do here if you want the lists in |
|
@tmilnthorp I pushed a possible fix for resetting global static state in tests. All tests are green locally now. If we are happy with the solution, we can make things internal, cleanup names, document better etc. |
|
@tmilnthorp I like where this is going, but a few observations:
I built on this and added Pull request with some changes on top of this branch: |
|
@tmilnthorp I understand you are busy these days, I'll merge this and work some more on it. |
|
@angularsen thanks! I’ve had this on my mind, just have not had time to come back to it recently. Hope to get back into some things in the coming months. |
|
@tmilnthorp No worries! 🙌 |
Added `UnitsNetSetup` to gather global state in a single place as a singleton, with the possibility of passing instances of it to static methods later to override the defaults. This way, state is no longer scattered among multiple classes that depend on each other in non-obvious ways and where initialization order has caused issues up until now. ### Changes - Add `UnitsNetSetup` to hold global state as a singleton property `Default` that controls default state initialization. - Add properties for all "services" that depend on global state: `UnitConverter, UnitAbbreviationsCache, UnitParser, QuantityParser` - Forward all other `Default` singleton properties from these services to `UnitsNetSetup.Default` and mark obsolete - Add some TODOs where it seems functionality is missing ### Testing ❌ Still running into a handful of flaky tests due to racing conditions. This was a regression in #1210, but still not fixed. Will investigate and fix in separate PR. ``` UnitAbbreviationsCacheTests.AllUnitsImplementToStringForInvariantCulture UnitAbbreviationsCacheTests.MapUnitToAbbreviation_AddCustomUnit_DoesNotOverrideDefaultAbbreviationForAlreadyMappedUnits ```
Fixes #1126
A quick draft to move the unit abbreviations into resource files. I also want to move methods for getting culture-specific abbreviations to the individual quantities.
This should:
TODOs
export DOTNET_SYSTEM_GLOBALIZATION_INVARIANT='1'System.Globalization.CultureNotFoundException UnitsNet.ToString() on system that Culture information is not available #1238