UnitsNet icon indicating copy to clipboard operation
UnitsNet copied to clipboard

Power is using the decimal type

Open tmilnthorp opened this issue 3 years ago • 6 comments

I'm upgrading to 5.1.0 in my projects and noticed that Power is using the decimal type.

I don't think this is a bug so much as questioning if this is expected? It has been this way since 2014, I only just noticed since the value property in v5 now returns the underlying type rather than always double.

I would expect this type to be consistent with much of the other types: double.

Per the PR:

I was thinking about this previously and I think decimal might be a better backing datatype because it can exactly represent powers of 10 because it's a decimal floating point type (double is binary floating point). It would make sense to use it since we're mainly concerned with SI units which are powers of 10, however it's a major breaking change and decimal is less efficient for calculations...

I don't think this is applicable anymore given the test checking within a tolerance. Let me make a PR and we can discuss,.

tmilnthorp avatar Feb 08 '23 14:02 tmilnthorp

I'm all for it, double down on double. BitRate, Information and Power are the types using decimal now. 3 out of 100+. It doesn't make any sense.

The only concern is breaking changes that requires another major version bump and that integer quantities like Information may start getting rounding errors it did not previously have.

Even so, a consistent type is better in the end and I would rather look into generics to support decimal, float and other number types if anyone cares enough about it to champion it.

angularsen avatar Feb 08 '23 18:02 angularsen

+1, the breaking changes with Power are preventing us from upgrading to v5 without lots of casting. I'd like to applaud @tmilnthorp for his PR efforts.

Muximize avatar Feb 21 '23 18:02 Muximize

I'd like to applaud @tmilnthorp for his PR efforts.

Thanks!

tmilnthorp avatar Feb 23 '23 14:02 tmilnthorp

@Muximize #1205 was resolved by #1207 , can you take it for a spin if this unblocks you?

Changing Power to double will be a breaking change, and you'll have to wait for v6 prerelease nugets for that. No ETA yet. Tracked by #1195 .

angularsen avatar Feb 26 '23 18:02 angularsen

Alas, we're using the conversion properties in quite a few places so we have to wait for v6.

I should have paid attention and protest against this change landing in v5 in the first place 😅

Muximize avatar Feb 27 '23 17:02 Muximize

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

stale[bot] avatar Jun 18 '23 14:06 stale[bot]

Fixed in #1195 for v6

angularsen avatar Jul 08 '24 13:07 angularsen