search

angularsen / UnitsNet

Makes life working with units of measurement just a little bit better.
https://www.nuget.org/packages/UnitsNet/
MIT No Attribution
2.65k stars 382 forks source link

🐛ComparisonType ignored in obsolete Equals method #1396

Closed lipchev closed 5 months ago

lipchev commented 5 months ago

fixes #1394

note that the error is also present in the v6 branch

angularsen commented 5 months ago

note that the error is also present in the v6 branch

I thought we removed these methods in v6? Aren't they marked obsolete?

lipchev commented 5 months ago

Doesn't seems like it- I only looked at my local version (with the fractions) and see that I've just removed the [Obsolete] annotation (as the operation would be safe), but hadn't noticed the other issue..

angularsen commented 5 months ago

Release UnitsNet/5.52.0 · angularsen/UnitsNet

lipchev commented 5 months ago

Doesn't seems like it- I only looked at my local version (with the fractions) and see that I've just removed the [Obsolete] annotation (as the operation would be safe), but hadn't noticed the other issue..

Actually I must have been looking at one of the other overloads, cause the [Obsolete] annotation is apparently still there and as looked at it a little more closely, realized that there is in fact a good reason to remove it: without knowing what unit the two quantities are in, there is no possible use of the absolute tolerance comparison 😄

The two overloads (if we need to have them inside the quantity) should be something like Equals(Density other, Density absoluteTolerance) and Equals(Density other, Ratio relativeTolerance)

angularsen commented 4 months ago

Will handle this in #1200 when reviewing all remaining [Obsolete] code. Added an action item for it.

lipchev commented 4 months ago

I have a working version of #1377 with 0 warning and 0 tests skipped (14 actually but those are expected), with all of the above mentioned changes as well as all [Obsolete] cases handled and ready for review. Everything is looking very good, but I just wanted to test out the nuget with my own code-base first. However I'm going to be busy this week so, decide it to postpone it for the next (weekend hopefully).

How do you want to handle it? All in one, or some part of it (like this PR) + the rest? I was hoping that we'd get at least some of these jsons merged (perhaps skipping those that cause a test to fail)- so that it would be easier to review my changes later on.

angularsen commented 4 months ago

OK I'll have to get into #1377 then, it's been looming over me for a long time. Started vacation now, so I may finally find some time for it.

lipchev commented 4 months ago

Yeah, but don't look at #1377, it's obsolete.. I'll have another one soon.

What i did for this particular [Obsolete] Equals overload was to removed it- as introducing the overload with Ratio would have introduced an ambiguity within the Ratio quantity and I didn't want to deal with that.

The user can either use the other overload (with the absolute tolerance) by multiplying the quantity by a ratio, or switch to directly using the QuantityComparison class.

angularsen commented 4 months ago

OK, let me know when you have something I can look at