Replace As(UnitSystem) and ToUnit(UnitSystem) with extension methods

[lipchev]

• refactored the As(UnitSystem) and ToUnit(UnitSystem) methods as extensions
• removed the As(UnitSystem) method from the IQuantity interface

[lipchev]

@angularsen In v5 the IQuantity only has one As(UnitSystem) and two ToUnit(UnitSystem) methods: one is returning an IQuantity and the other an IQuantity<TUnit>.

So while the new As(UnitSystem) extension method is a direct replacement for the As from the interface, the ToUnit(UnitSystem) only works with IQuantityOfType<TQuantity>, returning TQuantity - therefore I've left the ToUnit overloads on the interface unchanged.

In #1587 we talked about removing/deprecating both of them (and more) in v6, relying on the UnitConverter for the not-so-strongly-typed -conversions.

In the 🐲 PR I have them marked as [Obsolete] with a default interface implementation for net8+ and an explicit implementation for netstandard.

[angularsen]

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.
I like moving towards the quantity as a dumb value holder, with extension methods for syntax convenience and Quantity and UnitsNetSetup for access to functionality.

[angularsen]

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.

[angularsen]

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+.

[lipchev]

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?

[angularsen]

@lipchev I pushed another related refactor moving the untyped IQuantity ToUnit(UnitSystem) out of IQuantity and into an extension method, please see 79570f5 (#1600) if you agree.

[lipchev]

@lipchev I pushed another related refactor moving the untyped IQuantity ToUnit(UnitSystem) out of IQuantity and into an extension method, please see 79570f5 (#1600) if you agree.

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 ToUnit that would look really nasty. We could move them to some sort of an InterfaceQuantityExtensions, perhaps even in a separate namespace, but even so- I'd suggest using the UnitConverterter instead.

[angularsen]

I'm happy as long as it's not in IQuantity, but I do think extension methods are more discoverable than having to know to use UnitConverter. Previously, a consumer would call myQuantity.ToUnit() regardless of it being a concrete type or IQuantity or what have you, and an extension method would work as before.

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 Quantity, others via UnitConverter, QuantityParser, UnitParser, or indirectly via UnitsNetSetup or even via QuantityInfo. In particular if only certain overloads have extension methods and others don't.

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.

[lipchev]

I'm happy as long as it's not in IQuantity, but I do think extension methods are more discoverable than having to know to use UnitConverter. Previously, a consumer would call myQuantity.ToUnit() regardless of it being a concrete type or IQuantity or what have you, and an extension method would work as before.

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

1. ToUnit(Enum) : that implies that we support converting Mass to VolumeUnit
2. IQuantity<TUnit> myQuantity = .. this is just a performance trap, which should be avoided by the proper use of generics
3. var shouldNotCompile = mass.As(theOtherParameter) this should not work with a concrete mass (IMO)

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.

No, this is not a bug- however I'd argue that the IQuantity overload are not relevant to the concrete quantities. Not that am using the UnitSystem overloads anywhere, but if I were - then I'd be frustrated if I had to hit the arrows 3 times every time I'm looking for the second overload. There would, theoretically also be the As(Enum) overload that's going to appear, where it wasn't showing up before.

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.

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).

[angularsen]

I'm not as bothered by it, I'm used to seeing overloads that are untyped to support those use cases.
However I get your point, it causes unnecessary noise in the intellisense, and less help from the compiler.

I think the solution is simple though.

Assuming untyped usage is for the minority of consumers, let's just add an Untyped suffix to the relevant overloads. E.g. ToUnitUntyped() and AsUntyped(). Then it's still discoverable, but we get a cleaner intellisense for the majority of use cases.

This how ToUnit() currently looks like in Rider:

Both AI suggestion and ctrl+space offers relevant values to insert as argument:

With the untyped suffix, it looks like this:

IQuantity myQuantity = .. this is just a performance trap, which should be avoided by the proper use of generics

In the perspective of introducing fractions of BigInteger, I suspect this will be neglible.

However, a relevant point that I wanted to bring up separately is that I think we have way too many layers of interfaces now. It feels overly complicated and I'm not so sure it brings any real value. I haven't tried to tug at it yet, but it feels like we should be able to simplify a bit:

• Keep
  • Untyped IQuantity
  • Fully typed IQuantity<TSelf, TUnitType>, ILinearQuantity<TSelf, TUnitType>, ILogarithmicQuantity<TSelf, TUnitType>, IAffineQuantity<TSelf, TUnitType, TOffset>
• Remove
  • Interfaces with only TSelf, e.g. IQuantityOfType<TSelf>, ILinearQuantity<TSelf>
  • Interfaces with only TUnit, e.g. IQuantity<TUnitType>

This is worth a separate discussion though, let's circle back to it later and not slide off topic here.

[lipchev]

I'm not as bothered by it, I'm used to seeing overloads that are untyped to support those use cases. However I get your point, it causes unnecessary noise in the intellisense, and less help from the compiler.

I think the solution is simple though.

Assuming untyped usage is for the minority of consumers, let's just add an Untyped suffix to the relevant overloads. E.g. ToUnitUntyped() and AsUntyped(). Then it's still discoverable, but we get a cleaner intellisense for the majority of use cases.

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.

IQuantity myQuantity = .. this is just a performance trap, which should be avoided by the proper use of generics

In the perspective of introducing fractions of BigInteger, I suspect this will be neglible.

I wouldn't bet on it- the BigInteger only gets an allocation if bigger than int.MaxValue.

However, a relevant point that I wanted to bring up separately is that I think we have way too many layers of interfaces now. It feels overly complicated and I'm not so sure it brings any real value. I haven't tried to tug at it yet, but it feels like we should be able to simplify a bit:

* Keep
* Untyped `IQuantity`
* Fully typed `IQuantity<TSelf, TUnitType>`, `ILinearQuantity<TSelf, TUnitType>`, `ILogarithmicQuantity<TSelf, TUnitType>`, `IAffineQuantity<TSelf, TUnitType, TOffset>`
* Remove
* Interfaces with only `TSelf`, e.g. `IQuantityOfType<TSelf>`, `ILinearQuantity<TSelf>`
* Interfaces with only `TUnit`, e.g. `IQuantity<TUnitType>`

This is worth a separate discussion though, let's circle back to it later and not slide off topic here.

I don't know- IQuantity<TUnitType> is useful as a constraint for things related the the unit abbreviations, while the IQuantityOfType<TQuantity> is required for all types of extensions that deal with unit conversions (like the Equals) but are not expected to define a second generic parameter for the TUnit.

[angularsen]

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).

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.

[angularsen]

I'll add the suffix to the one overload I moved though as a starting point.

I don't know- IQuantity is useful as a constraint for things related the the unit abbreviations, while the IQuantityOfType is required for all types of extensions that deal with unit conversions (like the Equals) but are not expected to define a second generic parameter for the TUnit.

Gotcha, and maybe you are right. I'll have to spend more time with it to get a better feel for it.

[angularsen]

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 mean, sure we could, but aren't we talking like a handful of extension methods to bring methods out of IQuantity and into extension methods? We'll touch back on it.

[lipchev]

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 mean, sure we could, but aren't we talking like a handful of extension methods to bring methods out of IQuantity and into extension methods? We'll touch back on it.

Depends how you count it.. Here are my candidates for such a nugget:

• QuantityValue As(Enum)
• IQuantity ToUnit(Enum)
• IQuantity ToUnit(UnitSystem)
• IQuantity ToUnit(Enum, UnitConverter) (I don't know if we want the UnitConverter overloads- we currently only have it on the concrete type)
• IQuantity<TUnit> ToUnit(TUnit)
• IQuantity<TUnit> ToUnit(TUnit, UnitConverter)
• IQuantity<TUnit> ToUnit(UnitSystem)
• IQuantity<TUnit> ToUnit(UnitSystem, UnitConverter) : there isn't any such overload as of now, but you know... to me these all are all YAGNI anyway 😄
• bool Equals(IQuantity)
• bool Equals(IQuantity, IQuantity) (with tolerance)
• bool Equals<TUnit>(IQuantity<TUnit>)
• bool Equals<TUnit>(IQuantity<TUnit>, IQuantity<TUnit>)
• bool Equals<TQuantity>(IQuantity)
• bool Equals<TQuantity, TUnit>(IQuantity<TUnit>)

And whatever other crazy combinations I'm missing out 😄

[lipchev]

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).

[angularsen]

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 Untyped suffix and fewer breaking changes, but if we think it's a better compromise for the intellisense of concrete types, I'm fine with that too.

[lipchev]

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 mean, sure we could, but aren't we talking like a handful of extension methods to bring methods out of IQuantity and into extension methods? We'll touch back on it.

Depends how you count it.. Here are my candidates for such a nugget:

* `QuantityValue As(Enum)`
* `IQuantity ToUnit(Enum)`
* `IQuantity ToUnit(UnitSystem)`
* `IQuantity ToUnit(Enum, UnitConverter)` (I don't know if we want the `UnitConverter` overloads- we currently only have it on the concrete type)
* `IQuantity<TUnit> ToUnit(TUnit)`
* `IQuantity<TUnit> ToUnit(TUnit, UnitConverter)`
* `IQuantity<TUnit> ToUnit(UnitSystem)`
* `IQuantity<TUnit> ToUnit(UnitSystem, UnitConverter)` : there isn't any such overload as of now, but you know... to me these all are all YAGNI anyway 😄
* `bool Equals(IQuantity)`
* `bool Equals(IQuantity, IQuantity)` (with tolerance)
* `bool Equals<TUnit>(IQuantity<TUnit>)`
* `bool Equals<TUnit>(IQuantity<TUnit>, IQuantity<TUnit>)`
* `bool Equals<TQuantity>(IQuantity)`
* `bool Equals<TQuantity, TUnit>(IQuantity<TUnit>)`

And whatever other crazy combinations I'm missing out 😄

@angularsen I tried to create a separate class for the InterfaceQuantityExtensions , which I was gonna then move to a new project. I thought I'd try to keep the original interface signatures:

publicstaticIQuantity<TUnit>ToUnit<TUnit>(thisIQuantity<TUnit>quantity,UnitSystemunitSystem)whereTUnit:struct,EnumpublicstaticIQuantityToUnit(thisIQuantityquantity,UnitSystemunitSystem)

... but these now become ambiguous with the general extension:

publicstaticTQuantityToUnit<TQuantity>(thisTQuantityquantity,UnitSystemunitSystem)whereTQuantity: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 (QuantityExtensions and InterfaceQuantityExtensions), both extension methods are brought into scope at the same time (assuming both namespaces are imported), and the compiler can’t decide which one to pick.

Having the interface extensions named as ToUnitUntyped resolves the issue, but if we employed the same trick for the other interface members that would be a whole bunch of breaking changes (none for me 😆 ).

[lipchev]

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 (QuantityExtensions and InterfaceQuantityExtensions), both extension methods are brought into scope at the same time (assuming both namespaces are imported), and the compiler can’t decide which one to pick.

Having the interface extensions named as ToUnitUntyped resolves the issue, but if we employed the same trick for the other interface members that would be a whole bunch of breaking changes (none for me 😆 ).

@angularsen Ok if we ignore the Equals overloads, there are just these methods that we need to decide about (the rest of the interface members are covered without issues):

[Obsolete("This method will be removed from the interface in the next major update. Consider using the UnitConverter.Default.ConvertValue(quantity, unit) method instead.")]QuantityValueAs(Enumunit);[Obsolete("This method will be removed from the interface in the next major update. Consider using the UnitConverter.Default.ConvertTo(quantity, unit) method instead.")]IQuantityToUnit(Enumunit);[Obsolete("This method will be removed from the interface in the next major update. Consider using the UnitConverter.Default.ConvertTo(quantity, unit) method instead.")]IQuantityToUnit(UnitSystemunitSystem);[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(TUnitTypeunit);[Obsolete("This method will be removed from the interface in the next major update. Consider using the UnitConverter.Default.ConvertToUnit(quantity, unit) method instead.")]newIQuantity<TUnitType>ToUnit(UnitSystemunitSystem);

The way I see it there are just 3 options:

1. Make these obsolete in v6 and provide a default interface implementation (i.e. implement directly on the interface): this is what I've done in the 🐲 PR. If the user insists on using these, they have the time to create their own extensions.
2. Preserve the signatures and refactor as extensions: this means that all these should be implemented inside the QuantityExtensions and would be visible on the concrete types. I'm fine with this, as long as we keep the [Obsolete] attribute.
3. Extract into a separate nugget and change the names: this assumes that we'd want to maintain these extensions long-term. Doing this right away (in v6) would probably break some eggs, so personally I would suggest doing this as a follow up to 1) or 2), but that's up to you..

[lipchev]

@angularsen Bonus options:

a)

publicstaticIQuantityToUnit<TQuantity>(thisTQuantityquantity,UnitSystemunitSystem)whereTQuantity:class,IQuantity

b)

publicstaticIQuantity<TUnit>ToUnit<TQuantity,TUnit>(thisTQuantityquantity,UnitSystemunitSystem)whereTUnit:struct,EnumwhereTQuantity:class,IQuantity<TUnit>

None of these would show up for the Mass and would work for IQuantity / IQuantity<MassUnit> but we can't have both a) and b) using the ToUnit name, as they become ambiguous with each other 😄. Also I'm not really sure what happens if we implemented HowMuch as a class.

[lipchev]

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 (QuantityExtensions and InterfaceQuantityExtensions), both extension methods are brought into scope at the same time (assuming both namespaces are imported), and the compiler can’t decide which one to pick.
Having the interface extensions named as ToUnitUntyped resolves the issue, but if we employed the same trick for the other interface members that would be a whole bunch of breaking changes (none for me 😆 ).

@angularsen Ok if we ignore the Equals overloads, there are just these methods that we need to decide about (the rest of the interface members are covered without issues):

[Obsolete("This method will be removed from the interface in the next major update. Consider using the UnitConverter.Default.ConvertValue(quantity, unit) method instead.")]QuantityValueAs(Enumunit);[Obsolete("This method will be removed from the interface in the next major update. Consider using the UnitConverter.Default.ConvertTo(quantity, unit) method instead.")]IQuantityToUnit(Enumunit);[Obsolete("This method will be removed from the interface in the next major update. Consider using the UnitConverter.Default.ConvertTo(quantity, unit) method instead.")]IQuantityToUnit(UnitSystemunitSystem);[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(TUnitTypeunit);[Obsolete("This method will be removed from the interface in the next major update. Consider using the UnitConverter.Default.ConvertToUnit(quantity, unit) method instead.")]newIQuantity<TUnitType>ToUnit(UnitSystemunitSystem);

The way I see it there are just 3 options:

1. Make these obsolete in `v6` and provide a default interface implementation (i.e. implement directly on the interface): this is what I've done in the 🐲 PR. If the user insists on using these, they have the time to create their own extensions.
2. Preserve the signatures and refactor as extensions: this means that all these should be implemented inside the `QuantityExtensions` and would be visible on the concrete types. I'm fine with this, as long as we keep the `[Obsolete]` attribute.
3. Extract into a separate nugget and change the names: this assumes that we'd want to maintain these extensions long-term. Doing this right away (in `v6`) would probably break some eggs, so personally I would suggest doing this as a follow up to 1) or 2), but that's up to you..

Actually, I just realized there is another inconvenience with 1) - when working within a generic context where TQuantity : IQuantity<TQuantity, TUnit>, IComparable<TQuantity> (that's my use case) I'd have to use UnitConversionExtensions.ToUnit(value, unit) because the [Obsolete] interface function is now visible, and takes precedence over the extension method.

As such, I think I'm now more leaning towards 2) or 2) + 3).

[lipchev]

Here's what option 2) looks like in the 🐲 PR:

Notice that because the overloads are sorted alphabetically, the actual overload that we're calling isn't even visible anymore.

Things aren't so bad when we look at the Area:

But again, even if we had tagged the Untyped suffix in the end, that still wouldn't be that great.