Replace ToUnit(UnitSystem) from the IQuantity<TUnit> interface with an extension method #1606
Conversation
…tensions - removed the As(UnitSystem) method from the IQuantity interface
Split tests into ToUnit vs ToUnitTyped Update calls to ToUnitTyped where output is IQuantity.
…interface with an extension method ([Obsolete]) - renamed the ToUnitUntyped extension back to ToUnit and made it [Obsolete] - replaced the generated UnitSystem tests for the IQuantity / IQuantity<TUnit> with a single set of tests in the IQauntityTests
|
@angularsen This corresponds to option 2) from this comment: #1600 (comment) |
| public void ToUnit_UnitSystem_ReturnsValueInDimensionlessUnit() | ||
| {{ | ||
| Assert.Multiple(() => | ||
| {{ | ||
| var quantity = new {_quantity.Name}(value: 1, unit: {_baseUnitFullName}); | ||
| {_quantity.Name} convertedQuantity = quantity.ToUnit(UnitSystem.SI); | ||
| Assert.Equal({_baseUnitFullName}, convertedQuantity.Unit); | ||
| Assert.Equal(quantity.Value, convertedQuantity.Value); | ||
| }}, () => | ||
| {{ | ||
| IQuantity<{_unitEnumName}> quantity = new {_quantity.Name}(value: 1, unit: {_baseUnitFullName}); | ||
| IQuantity<{_unitEnumName}> convertedQuantity = quantity.ToUnit(UnitSystem.SI); | ||
| Assert.Equal({_baseUnitFullName}, convertedQuantity.Unit); | ||
| Assert.Equal(quantity.Value, convertedQuantity.Value); | ||
| }}); | ||
| }} |
There was a problem hiding this comment.
I've only kept the concrete part of this test, but on second thought there shouldn't really be any need for it to exist here either- as we already have tests for the UnitSystem constructor.
In case we decide to move forward with the removal of the interface methods from the interface in v6, then these tests should probably go to another test file- something like QuantityExtensionsTests.
One point I forgot to mention in the other thread was the requirement for So, there you have it, another thing for you to consider (I can't say I have any preferences- one way or the other). |
|
@claude Review the PR |
|
Claude encountered an error —— View job I'll analyze this and get back to you. |
|
Claude code does not yet work with PRs from forks, here is a local review: Code Review for PR #1606: Replace ToUnit(UnitSystem) from IQuantity interface with extension method 📋 Summary This PR refactors the ToUnit(UnitSystem) method by moving it from the IQuantity interface to extension methods,
✅ Style and Conventions
🏗️ Code Quality and Architecture Improvements:
Implementation details: // Before (in interface)
IQuantity<TUnitType> ToUnit(UnitSystem unitSystem);
// After (as extension)
[Obsolete("This method will be removed from the interface in the next major update.")]
public static IQuantity<TUnit> ToUnit<TUnit>(this IQuantity<TUnit> quantity, UnitSystem unitSystem)🧪 Test Coverage
🔍 Potential Issues
🚀 Performance No performance impact - implementation logic unchanged, just relocated. 💡 Suggestions
// Old (won't compile after removal)
IQuantity<LengthUnit> length = Length.FromMeters(1);
var converted = length.ToUnit(UnitSystem.SI);
```cs
// New (requires using UnitsNet;)
using UnitsNet; // Add this
IQuantity<LengthUnit> length = Length.FromMeters(1);
var converted = length.ToUnit(UnitSystem.SI); // Now works as extension✅ Verdict Approved with minor suggestions. This is a well-executed refactoring that significantly improves code maintainability. The |
2 participants
ToUnit(UnitSystem)overload from theIQuantity<TUnit>interface with an extension method ([Obsolete])ToUnitUntypedextension back toToUnitand made it[Obsolete]UnitSystemtests for theIQuantity/IQuantity<TUnit>with a single set of tests in theIQauntityTests