Skip to content

Conversation

lipchev
Copy link
Collaborator

@lipchev lipchev commented Aug 9, 2025

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

…tensions

- removed the As(UnitSystem) method from the IQuantity interface
@lipchev
Copy link
Collaborator Author

lipchev commented Aug 9, 2025

@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
Copy link
Owner

angularsen commented Aug 9, 2025

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.

@@ -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);
Copy link
Owner

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

Copy link
Owner

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

Copy link
Collaborator Author

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?

@angularsen
Copy link
Owner

@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
Copy link
Collaborator Author

lipchev commented Aug 9, 2025

@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
Copy link
Owner

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
Copy link
Collaborator Author

lipchev commented Aug 10, 2025

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
Copy link
Owner

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:
image

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

With the untyped suffix, it looks like this:
image

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
Copy link
Collaborator Author

lipchev commented Aug 10, 2025

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
Copy link
Owner

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
Copy link
Owner

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
Copy link
Owner

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
Copy link
Collaborator Author

lipchev commented Aug 10, 2025

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 😄

Split tests into ToUnit vs ToUnitTyped
Update calls to ToUnitTyped where output is IQuantity.
@lipchev
Copy link
Collaborator Author

lipchev commented Aug 10, 2025

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 angularsen mentioned this pull request Aug 10, 2025
25 tasks
@angularsen
Copy link
Owner

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.

@angularsen angularsen enabled auto-merge (squash) August 10, 2025 16:23
@angularsen angularsen merged commit be9c772 into angularsen:master Aug 10, 2025
1 check passed
@lipchev
Copy link
Collaborator Author

lipchev commented Aug 16, 2025

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:

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 (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
Copy link
Collaborator Author

lipchev commented Aug 16, 2025

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.")]
        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:

  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
Copy link
Collaborator Author

lipchev commented Aug 16, 2025

@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 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
Copy link
Collaborator Author

lipchev commented Aug 18, 2025

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.")]
        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:

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
Copy link
Collaborator Author

lipchev commented Aug 18, 2025

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

image

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:

image

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants