feat: impl Add, Sub, Mul, Div for all newtypes. #317
Conversation
| ($type: ty, $fn_name: ident, $a:expr,$b:expr) => { | ||
| #[test] | ||
| fn $fn_name() { | ||
| let a = $a; |
There was a problem hiding this comment.
Overall, this looks great -- thanks!
I have some concerns with the tests
- For some of the types, 0.0 is not allowed. For example, Time can be 0 but DemeSize cannot. That case is not covered here.
- One way to stress test these operations in more depth is via proptest. We could add that as a dev dependency and then let it test all sorts of edge cases for the input into these ops. The simplest method is to use their macro to do the heavy lifting. The idea would be to let proptest generate two f64. We can then say
let t = DemeSize::try_from(a + b). Ift.is_ok(), then we also expect DemeSize(a) + b, a + DemeSize(b), etc., to all be Some(...) and to contain the same value.
There was a problem hiding this comment.
Thanks, I'll have a look into proptest when I have time. That sounds like a good solution.
There was a problem hiding this comment.
There are limitations to property testing. Upon a bit of reflection, It may be best now to manually treat edge cases: NaN input (f64::NAN), 0 for DemeSize, etc..
There was a problem hiding this comment.
yes can have known edge cases in and additionally property testing.
There was a problem hiding this comment.
@alxsimon -- I am happy to merge this and revisit detailed testing later. Overall, these crates could be doing a better job testing the newtypes throughout. I think it'll be more efficient to do a higher level review of the testing and make changes later.
There was a problem hiding this comment.
Feel free to merge this one, good for me to come back to tests later
Closes #277