Add statically-typed UnitInfo classes #1024
Conversation
…uantityInfo constructor call
| baseUnits.N != null ? $"amount: AmountOfSubstanceUnit.{baseUnits.N}" : null, | ||
| baseUnits.J != null ? $"luminousIntensity: LuminousIntensityUnit.{baseUnits.J}" : null | ||
| }.Where(str => str != null)); | ||
| BaseUnit = {_unitEnumName}.{_quantity.BaseUnit}; |
There was a problem hiding this comment.
I initialized all static properties in the constructor now to guarantee order
| #region Static Properties | ||
|
|
||
| /// <inheritdoc cref=""IQuantity.QuantityInfo""/> | ||
| public static QuantityInfo<{_unitEnumName}> Info {{ get; }} | ||
| public static {_quantity.Name}.{_quantity.Name}QuantityInfo Info {{ get; }} |
There was a problem hiding this comment.
Now have a per-quantity info class that derives from QuantityInfo
| }, | ||
| Acceleration.BaseUnit, Acceleration.Zero, Acceleration.BaseDimensions, QuantityType.Acceleration) | ||
| { | ||
| CentimeterPerSecondSquared = new UnitInfo<AccelerationUnit>(AccelerationUnit.CentimeterPerSecondSquared, "CentimetersPerSecondSquared", BaseUnits.Undefined); |
There was a problem hiding this comment.
I know these lines are a bit redundant from above. But constructing it all at once involves moving some QuantityInfo constructors around due to nullability checks. I will consider this in a different PR (probably expose a ICollection<UnitInfo> instead of a straight array).
Codecov Report
@@ Coverage Diff @@
## master #1024 +/- ##
========================================
- Coverage 82.9% 81.0% -1.8%
========================================
Files 309 309
Lines 46926 50646 +3720
========================================
+ Hits 38865 41009 +2144
- Misses 8061 9637 +1576
Continue to review full report at Codecov.
|
|
|
||
| /// <summary> | ||
| /// </summary> | ||
| public sealed class AngleQuantityInfo : QuantityInfo<AngleUnit> |
There was a problem hiding this comment.
Although I like the idea, I'm hesitant to generate anything extra involving our 1000+ units.
The binary size is in my opinion already bloated, and this PR increases it from 1,7 to 1,9 MB. I don't think static unit info properties add enough value to be worth the extra bytes.
Can we instead make it easier to look up by unit enum, if we don't already have such a method?
That way we no longer need the 100+ extra quantity info types either since there are no quantity-specific members left.
There was a problem hiding this comment.
Although I like the idea, I'm hesitant to generate anything extra involving our 1000+ units. The binary size is in my opinion already bloated, and this PR increases it from 1,7 to 1,9 MB. I don't think static unit info properties add enough value to be worth the extra bytes.
Can we instead make it easier to look up by unit enum, if we don't already have such a method?
That way we no longer need the 100+ extra quantity info types either since there are no quantity-specific members left.
I hear that. It does add quite a bit of size.
My thought process is that since Enum is not extensible, we need to use UnitInfo. When we do add better extensibility (per #865), I envision conversion to look something like:
public Acceleration ToUnit(UnitInfo unit)
{
// Do conversion here
return new Acceleration...
}That would allow us to do something similar as now:
var acceleration = Acceleration.FromMetersPerSecondSquared( 1.0 );
var converted = acceleration.ToUnit( Acceleration.Info.FootPerSecondSquared );And when someone can add an Acceleration unit at runtime in the hopefully near-future:
public enum CustomAccelerationUnits
{
WarpSpeed
}
var warpSpeedUnit = new UnitInfo( CustomAccelerationUnits.WarpSpeed, "WarpSpeedz", new BaseUnits( LengthUnit.LightYear ) );
var customAccelerationUnit = Acceleration.Info.AddUnit( warpSpeedUnit );
var acceleration = Acceleration.FromMetersPerSecondSquared( 1.0 );
var warpSpeed = acceleration.ToUnit( warpSpeedUnit );But indeed, perhaps I can add just a dictionary lookup for this. Let me come back to this.
There was a problem hiding this comment.
BTW, speaking of overall size, how do you feel about removing the auto-generated properties and FromXXX methods? In today's API, ToUnit can be passed in the enum and provides everything the generated properties do. In the future: ToUnit(UnitInfo)?
There was a problem hiding this comment.
Yes, you are right. Let's do something extensible other than enum. I like the idea of being able to add UnitInfo and later convert with it.
The FromX factory methods is something I do like myself, because it is how I primarily construct quantities and it is very much in your face how to use it when exploring. It is also similar to TimeSpan that way.
If we were to get serious in trimming down size, I would definitely remove it, but I think it would hurt syntax and discoverability too much to remove them from the "main" library. We could always offer leaner alternatives for those who care about size, then we could be less careful in the full library, but I honestly don't want to maintain more things :-D I struggle to find time as it is.
You can add static using statements to reduce Length.From(LengthUnit.Meter) to Length.From(Meter), but you would have to manually set this up everywhere you use it so it is still cumbersome. The number extensions nuget allow you to do 1.Meters(), which is nice, but I have rarely used it myself.
There was a problem hiding this comment.
I would even argue about introducing a WarpUnitInfo that has to also override the following two abstract methods from UnitInfo:
QuantityValue FromBaseUnit(QuantityValue baseUnitValue);QuantityValue ToBaseUnit(QuantityValue baseUnitValue);
I personally don't see any benefit from having the XXXUnitEnum over a XXXQuantityInfo (extending QuantityInfo) that has the list of UnitInfo<XXX> (the generic type parameter is optional), as well as individual properties for each unit (this time using the concrete UnitInfo type).
I don't think this would have a big impact on size - as we're simply substituting one UnitInfo<AUnit> with the corresponding AUnitInfo and replacing one Enum type with one Class type.
This would add the following benefits:
- remove the need to do any case-switching while converting (should have a significant impact on both size and performance)
- make it very easy to extend quantities with missing units (
Quantity.From(Enum)would no longer be a concern - so the Serializers should work out-of-the box) - make it very easy to create custom quantities (should also work for serialization, UnitSystem, UnitMath etc.)
- remove the need for an external converter class (we could have a one-line static method in the quantity itself)
- give us a way to customize the type of QuantityValue that is constructed (thinking of it as a sort of factory)
|
Closing in favor of a lookup based-mechanism |
For getting a particular
UnitInfo, we currently have to do a search based on name or enum value. This adds some typed convenience helpers: