Skip to content
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

🐛ComparisonType ignored in obsolete Equals method #1396

Merged
merged 2 commits into from
Jun 16, 2024

Conversation

lipchev
Copy link
Collaborator

@lipchev lipchev commented Jun 16, 2024

fixes #1394

note that the error is also present in the v6 branch

@angularsen
Copy link
Owner

note that the error is also present in the v6 branch

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

@angularsen angularsen changed the title ComparisonType ignored in the (obsolete) Equals method 🐛ComparisonType ignored in obsolete Equals method Jun 16, 2024
@angularsen angularsen enabled auto-merge (squash) June 16, 2024 11:56
@lipchev
Copy link
Collaborator Author

lipchev commented Jun 16, 2024

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 angularsen merged commit 4045199 into angularsen:master Jun 16, 2024
1 check passed
@angularsen
Copy link
Owner

@lipchev
Copy link
Collaborator Author

lipchev commented Jun 16, 2024

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 angularsen mentioned this pull request Jul 8, 2024
13 tasks
@angularsen
Copy link
Owner

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

@lipchev
Copy link
Collaborator Author

lipchev commented Jul 8, 2024

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

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

lipchev commented Jul 8, 2024

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

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

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

Successfully merging this pull request may close these issues.

QuantityGenerator ignores ComparisonType in the obsolete Equals method
2 participants