💥Require unit enum types to also be struct #1472
Conversation
Change all `where TUnitType : Enum` generic type constraints to include `struct` constraint, to avoid usages like this.
```cs
UnitsNetSetup.Default.UnitParser.Parse<LengthUnit>("abc"); // Ok
UnitsNetSetup.Default.UnitParser.Parse<Enum>("abc"); // Compiles, but throws exception
```
This also fixes inconsistency, between `UnitAbbreviationsCache.GetUnitAbbreviations<TUnitType>` and `UnitAbbreviationsCache.GetDefaultAbbreviation<TUnitType>`.
### Changes
- Add `struct` constraint to all `Enum` constraints
- Remove two test cases `MapAndLookup_MapWithSpecificEnumType_LookupWithEnumType`, `MapAndLookup_WithEnumType`
### Noteworthy
There are some minor use cases for passing non-generic `Enum` value, like getting unit abbreviations given a `IQuantity` such as `UnitAbbreviations.GetDefaultAbbreviation(quantity.Unit)`. Now this requires using `GetDefaultAbbreviation(quantity.Unit.GetType(), Convert.ToInt32(quantity.Unit))` instead. However, `GetAbbreviations()` already had this requirement, so now it's more consistent.
### Sources
https://stackoverflow.com/a/74773273
https://devblogs.microsoft.com/premier-developer/dissecting-new-generics-constraints-in-c-7-3/
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## release/v6 #1472 +/- ##
==========================================
Coverage 86% 86%
==========================================
Files 298 298
Lines 30515 30515
==========================================
Hits 26405 26405
Misses 4110 4110 ☔ View full report in Codecov by Sentry. |
| /// <returns>Unit abbreviations associated with unit.</returns> | ||
| public string[] GetUnitAbbreviations<TUnitType>(TUnitType unit, IFormatProvider? formatProvider = null) where TUnitType : Enum | ||
| public string[] GetUnitAbbreviations<TUnitType>(TUnitType unit, IFormatProvider? formatProvider = null) where TUnitType : struct, Enum | ||
| { | ||
| return GetUnitAbbreviations(typeof(TUnitType), Convert.ToInt32(unit), formatProvider); | ||
| } |
There was a problem hiding this comment.
This is great, introducing the struct constraint here opens up the possibility for using
Unsafe.As<TUnit, int>(ref unit)
instead of
Convert.ToInt32(unit)
which is ~34x faster on net8.0 and ~92x faster on net48 🚀
There was a problem hiding this comment.
This one is really bugging me with it's 0% coverage - I'm definitely going to do something about it soon..
There was a problem hiding this comment.
If we want to be really strict about it, we should consider making the QuantityInfo class abstract- this way we know that the user won't be able to implement IQuantity without having actually implemented IQuantity<TUnit>.
lipchev
left a comment
lipchev
left a comment
There was a problem hiding this comment.
This all looks great. Just one question - do you have a preference with the way that the generic constraints are positioned (same line or new line)?
Great! No, I'm too old to care about these things anymore 😅 Yolo! |
…traint # Conflicts: # UnitsNet.Tests/UnitAbbreviationsCacheTests.cs
Change all
where TUnitType : Enumgeneric type constraints to includestructconstraint, to avoid usages like this.This also fixes inconsistency, between
UnitAbbreviationsCache.GetUnitAbbreviations<TUnitType>andUnitAbbreviationsCache.GetDefaultAbbreviation<TUnitType>.Changes
structconstraint to allEnumconstraintsMapAndLookup_MapWithSpecificEnumType_LookupWithEnumType,MapAndLookup_WithEnumTypeNoteworthy
There are some minor use cases for passing non-generic
Enumvalue, like getting unit abbreviations for anIQuantitysuch asUnitAbbreviations.GetDefaultAbbreviation(quantity.Unit).After this PR, you now have to do
GetDefaultAbbreviation(quantity.Unit.GetType(), Convert.ToInt32(quantity.Unit))instead.GetAbbreviations()already had this requirement, so now it's more consistent at least.Sources
https://stackoverflow.com/a/74773273
https://devblogs.microsoft.com/premier-developer/dissecting-new-generics-constraints-in-c-7-3/