-
Notifications
You must be signed in to change notification settings - Fork 397
Replace As(UnitSystem)
and ToUnit(UnitSystem)
with extension methods
#1600
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
…tensions - removed the As(UnitSystem) method from the IQuantity interface
@angularsen In v5 the So while the new In #1587 we talked about removing/deprecating both of them (and more) in v6, relying on the In the 🐲 PR I have them marked as [Obsolete] with a default interface implementation for net8+ and an explicit implementation for netstandard. |
Yeah I think it's better to move this responsibility out of the quantity and into UnitConverter. There's too much stuff to implement for a custom quantity. |
@@ -999,13 +987,13 @@ IQuantity IQuantity.ToUnit(Enum unit) | |||
} | |||
|
|||
/// <inheritdoc /> | |||
IQuantity IQuantity.ToUnit(UnitSystem unitSystem) => ToUnit(unitSystem); | |||
IQuantity IQuantity.ToUnit(UnitSystem unitSystem) => this.ToUnit(unitSystem); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's use static method invocation syntax QuantityExtensions.ToUnit(unitSystem)
instead of extension method syntax.
I think it can be confusing to read extension method usage in general, and maybe in particular with this.
from within a member of the type that is extended.
/// <param name="unitSystem">The <see cref="UnitSystem"/> to convert the quantity value to.</param> | ||
/// <returns>The converted value.</returns> | ||
double As(UnitSystem unitSystem); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the ToUnit(UnitSystem) only works with IQuantityOfType, returning TQuantity - therefore I've left the ToUnit overloads on the interface unchanged.
Ok, but we will eventually move all the As()
and ToUnit()
methods to extension methods, right?
Also, I believe we can use extension methods instead of default interface bodies to support both netstandard and net8+.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not all the methods: I don't plan to support extension methods that return IQuantity
or IQuantity<TUnit>
, just TQuantity
.
Here are all the extension methods:
https://github.com/lipchev/UnitsNet/blob/fractional-quantity-value/UnitsNet/Extensions/UnitConversionExtensions.cs
PS In the other PR these are still in the which I named as UnitConversionExtensions
- should I keep them there or move them to the QuantityExtensions
?
@lipchev I pushed another related refactor moving the untyped |
I don't like it- it's true that the right extension is being called for the concrete types, but the other overloads are still showing up. If we were to create 3 overloads for each |
I'm happy as long as it's not in I'm not sure I understand why having several overloads show up in the intellisense is a bad thing, as long as they are relevant and valid. Is this the Resharper bug that shows irrelevant overloads? If so that'll be fixed soon. As a consumer, it can be confusing that some stuff is accessible via It is getting a bit complicated, but I do think extension methods help with discoverability at least. We can get more a feel for it, but do you agree to at least move it out of IQuantity for now as an extension method? We can discuss further whether it should be moved elsewhere. |
In both cases we're removing it from the interface, its just a question whether these overloads are suitable for large enough use cases- I'd personally strongly discourage their use altogether
No, this is not a bug- however I'd argue that the
You've only replaced the one overload- was that intentional? Please tell me which overloads you'd like me to add (or add them yourself if you prefer). |
Fine, but if possible- it would be nice if we could move these out to a separate nugget (eventually). I really don't want to see these used anywhere in my code base.
I wouldn't bet on it- the
I don't know- |
Ah yes, I gave a quick try to move all the ToUnit overloads, but at first glance this required more work than just the untyped one. I'll merge this as-is, and we can iterate on it. |
I'll add the suffix to the one overload I moved though as a starting point.
Gotcha, and maybe you are right. I'll have to spend more time with it to get a better feel for it. |
I mean, sure we could, but aren't we talking like a handful of extension methods to bring methods out of |
Depends how you count it.. Here are my candidates for such a nugget:
And whatever other crazy combinations I'm missing out 😄 |
Split tests into ToUnit vs ToUnitTyped Update calls to ToUnitTyped where output is IQuantity.
Also, I'm sure you're aware that by renaming the method you're introducing a breaking change (for whoever might have been using the untyped overload). |
Yes I know, and since we're likely affecting a small minority, I'm fine with it. I added a bullet point to #1200 to include in changelog. In my opinion, we could keep it without the |
@angularsen I tried to create a separate class for the public static IQuantity<TUnit> ToUnit<TUnit>(this IQuantity<TUnit> quantity, UnitSystem unitSystem) where TUnit : struct, Enum
public static IQuantity ToUnit(this IQuantity quantity, UnitSystem unitSystem) ... but these now become ambiguous with the general extension: public static TQuantity ToUnit<TQuantity>(this TQuantity quantity, UnitSystem unitSystem) where TQuantity : IQuantityOfType<TQuantity> when the methods live in the same static class, overload resolution works fine because the compiler only considers overloads from a single candidate set. Once we split them into two static classes ( Having the interface extensions named as |
@angularsen Ok if we ignore the [Obsolete("This method will be removed from the interface in the next major update. Consider using the UnitConverter.Default.ConvertValue(quantity, unit) method instead.")]
QuantityValue As(Enum unit);
[Obsolete("This method will be removed from the interface in the next major update. Consider using the UnitConverter.Default.ConvertTo(quantity, unit) method instead.")]
IQuantity ToUnit(Enum unit) ;
[Obsolete("This method will be removed from the interface in the next major update. Consider using the UnitConverter.Default.ConvertTo(quantity, unit) method instead.")]
IQuantity ToUnit(UnitSystem unitSystem);
[Obsolete("This method will be removed from the interface in the next major update. Consider using the UnitConverter.Default.ConvertToUnit(quantity, unit) method instead.")]
IQuantity<TUnitType> ToUnit(TUnitType unit);
[Obsolete("This method will be removed from the interface in the next major update. Consider using the UnitConverter.Default.ConvertToUnit(quantity, unit) method instead.")]
new IQuantity<TUnitType> ToUnit(UnitSystem unitSystem); The way I see it there are just 3 options:
|
@angularsen Bonus options: a) public static IQuantity ToUnit<TQuantity>(this TQuantity quantity, UnitSystem unitSystem)
where TQuantity : class, IQuantity b) public static IQuantity<TUnit> ToUnit<TQuantity, TUnit>(this TQuantity quantity, UnitSystem unitSystem)
where TUnit : struct, Enum
where TQuantity : class, IQuantity<TUnit> None of these would show up for the |
Actually, I just realized there is another inconvenience with 1) - when working within a generic context As such, I think I'm now more leaning towards 2) or 2) + 3). |
As(UnitSystem)
andToUnit(UnitSystem)
methods as extensionsAs(UnitSystem)
method from theIQuantity
interface