Closed coolreader18 closed 3 years ago
Thanks for the PR!
I need to think about this one a bit though.
I like that When
goes back to only depending on <I,O>
rather than <R,I,O>
but I am concerned about:
PhantomData
. Now When
is tied to <I,O> not through a field that it needs but through a field that we are adding solely to satisfy typechecks. This makes it more implicit and easier to get wrong in the future. u64
rather than a function pointer. Something that I liked about the function pointer approach is that it enforced some amount of internal type safety. Because the ID
was: fn(R, I) -> O
, it verified that the mocks being passed to MockStore
were mocking the same inputs and same outputs. Now that the ID
is just a number, we have to make sure it is right ourselves. I might have more thoughts later but these are my initial gut reactions.
Something else I like about this:
I have been thinking for the past few weeks now that I should add support for function mocking, not just method mocking. i.e.,:
#[faux::mock]
pub fn some_method(a: i32) -> Vec<u32> {
/* ... */
}
#[test]
fn my_test {
when!(some_method).then_return(vec![3, 4, 5]);
assert_eq!(some_method(3), vec![3, 4, 5]);
}
This would require some sort of global shared knowledge and this PR sort of tips its toes into the idea of having something global across tests.
That being said, function mocking is in its super early stages, I haven't even sat down and thought of a good API for it yet, but it is something I want eventually.
After looking at this more and thinking about it over the weekend I have decided against merging this PR in its current state.
My main concerns are:
That being said, if a new bug arises that requires a new unique identifier I will definitely revisit this decision.
For now, I took a quick stab at what a more useful unique fn pointer would be: https://github.com/nrxus/faux/commit/093d8391d4fae0b00e1f3402fd5c249617ea4777
That work is incomplete as it would break support for mocking outputs that borrow data from self
(which is currently supported but marked as unsafe
). However, it might be a good place to look at if you are curious on another approach of making the unique ids more robust. I put that in a wip branch for now as it is not the important thing I want to work for faux at the moment but feel free to take a look at it anyway (:
As mentioned in #42. Not sure if the
R
type parameter inWhen
needed to be kept; it seemed like it wasn't strictly necessary even with the functions-as-ids so I was wondering if there was another purpose.