approx icon indicating copy to clipboard operation
approx copied to clipboard

Implement traits for `Option` and `Result`

Open skreborn opened this issue 5 years ago • 5 comments

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.

skreborn avatar Aug 31 '20 13:08 skreborn

@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 avatar Nov 14 '20 13:11 troublescooter

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.

troublescooter avatar Nov 14 '20 14:11 troublescooter

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?

t-rapp avatar Oct 21 '22 12:10 t-rapp

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.

evbo avatar Dec 07 '23 21:12 evbo