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

Implement traits for Option and Result #57

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

skreborn
Copy link

Values of types Option and Result are frequently checked for equality in testing, but approx does not currently implement its traits on these types. This PR aims to do just that.

For Result<T, E> I have opted to require the given trait on both T and E, so that the error can also contain approximately comparable values. I'm not entirely sure about this decision, however, so I would appreciate your input. The downside of this approach is that all error types are also forced to implement the given trait, which might be asking too much of the users of this library - especially since there is no easy way (e.g. #[derive(...)]) to do it automatically.

It's worth noting that (Result<T, E> as UlpsEq)::default_max_ulps returns the maximum tolerance of T and E. This is because the return type cannot be a tuple (unlike AbsDiffEq), and it seemed reasonable to allow the greater value. If anybody has a better idea - that doesn't involve breaking changes -, please let me know.

I can separate the implementations of Option and Result into separate PRs if necessary.

@troublescooter
Copy link

troublescooter commented Nov 14, 2020

@brendanzab
Any chance to get this reviewed and merged?

Much more useful would be to only need Result<T: *DiffEq, E>, with errors not being equal to each other.

@troublescooter
Copy link

troublescooter commented Nov 14, 2020

On second thought, this would disagree with the behaviour of PartialEq on Result, so it's not clear whether this suggestion would be too surprising. Maybe *DiffEq should not be implemented for Result.

I do think that the Option<T: *DiffEq> case is unsurprising and would be useful, which was what lead me to this PR.

Edits: Should have read the comment before checking out the commit, the problems with the bounds on E were already clearly stated, my bad.

@t-rapp
Copy link

t-rapp commented Oct 21, 2022

Stumbled over the missing implementation of Option<T>, too. Unfortunately it cannot manually be implemented outside of Approx as Option is a foreign type, and AbsDiffEq is a foreign trait to my module.

In my opinion implementation of Result is less import if Optionis available as you could then write your tests like

assert_abs_diff_eq!(x.ok(), Some(...));

So how about merging that part for the time being?

@evbo
Copy link

evbo commented Dec 7, 2023

Could this be added as a cargo feature, maybe marked as "experimental"? In my case I am comparing type Vec<Option<f64>> in my assertions so it's really convenient having that type supported out of the box.

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.

4 participants