Simplify interfaces#1662
Conversation
|
@claude Review idea in PR description and current code change |
|
Claude finished @angularsen's task in 4m 56s —— View job PR Review: Simplify interfaces
Design IdeaThe PR has two goals:
Both goals are reasonable. The first is clearly useful. The second reduces redundancy in the generated code significantly. What ChangedHierarchy restructure (non-generated files):
Generated quantities: Before (per-quantity flat list): public readonly partial struct Length :
IArithmeticQuantity<Length, LengthUnit>,
#if NET7_0_OR_GREATER
IDivisionOperators<Length, Length, double>,
IMultiplyOperators<Length, Length, Area>,
IComparisonOperators<Length, Length, bool>,
IParsable<Length>,
#endif
IComparable, IComparable<Length>, IEquatable<Length>, IFormattableAfter (clean hierarchy): public readonly partial struct Length : IQuantity<Length, LengthUnit>, ILinearQuantity<Length>
#if NET7_0_OR_GREATER
, IMultiplyOperators<Length, Length, Area>
...
#endifIssues1. Breaking:
|
4a7a992 to
bd39d28
Compare
|
@angularsen If it helps for reviewing I can split up this PR, keeping the simplifying here and make a separate PR for adding |
|
As I was refactoring the interfaces last time, I actually did pretty much the same thing - but in the end decided to revert it due to the following consideration: the |
bd39d28 to
5df56f4
Compare
|
I removed the |
|
@lipchev Yes indeed, moving interfaces up will make things harder for custom quantities. I was struggling a bit with |
|
@claude review |
|
Claude finished @angularsen's task in 3m 47s —— View job PR Review: Simplify interfaces (updated)
Scope has changed since the last reviewPer the discussion with @lipchev, the
So all four of my previously-raised breaking-change concerns (items 1–4 in the earlier review) no longer apply to the current diff. This is now a much narrower, lower-risk change purely about the interface hierarchy for What's left in scope
Remaining considerations
Verification
OverallThe direction is good and the risk profile dropped significantly once the |
| /// </remarks> | ||
| public static TQuantity Sum<TSource, TQuantity, TUnit>(this IEnumerable<TSource> source, Func<TSource, TQuantity> selector, TUnit targetUnit) | ||
| where TQuantity : ILogarithmicQuantity<TQuantity, TUnit> | ||
| where TQuantity : ILogarithmicQuantity<TQuantity>, IQuantity<TQuantity, TUnit> |
There was a problem hiding this comment.
I suggest keeping the two-parameter quantity interfaces, if viable.
I think this is simpler for consumers: they can choose between the unit-agnostic and unit-specific interfaces, instead of repeating both constraints in their own APIs:
where TQuantity : ILogarithmicQuantity<TQuantity>, IQuantity<TQuantity, TUnit>A composite interface captures that relationship directly:
public interface ILogarithmicQuantity<TSelf, TUnit> :
IQuantity<TSelf, TUnit>,
ILogarithmicQuantity<TSelf>
where TSelf : ILogarithmicQuantity<TSelf, TUnit>
where TUnit : struct, Enum
{
}This matches the existing shape of the quantity interfaces (before the PR):
IArithmeticQuantity<TSelf, TUnit>
IAffineQuantity<TSelf, TUnit, TOffset>
ILogarithmicQuantity<TSelf, TUnit>To me, this represents a useful concept: “a logarithmic quantity with a known unit enum”, while avoiding repeated multi-constraint combinations in public APIs.
There was a problem hiding this comment.
Actually, this goes for all the interfaces; affine, logarithmic, arithmetic.
Can we preserve the two-parameter composite interfaces to avoid consumers having to specify multiple generic constraints?
There was a problem hiding this comment.
I pushed fix that I think works, take a look if you agree. I tried to minimize diff in PR also, we can refactor later.
3 participants
In my use of UnitsNet, I find the recently added
IQuantityOfType<TQuantity>very useful. I wanted to do a comparison between some, but could not without addingIComparisonOperators<TQuantity, TQuantity, bool>to the constraints of my method. I then realized this can be added to the base interface instead of the quantity itself.While doing this I found a way to simplify some other interfaces in the inheritance chain, like reusing
IArithmeticQuantityfor bothILinearQuantityandILogarithmicQuantityand removing one level of inheritance.@lipchev What are your thoughts? Is this an improvement?