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

Common default return value of functions with generics #648

Closed
balazsbonis opened this issue Apr 11, 2016 · 27 comments · Fixed by #764
Closed

Common default return value of functions with generics #648

balazsbonis opened this issue Apr 11, 2016 · 27 comments · Fixed by #764

Comments

@balazsbonis
Copy link
Collaborator

I am writing unit tests for little tasks that are talking to the database. My test cases also include scenarios where a given object is not found in the database, and therefore I'd like to emulate that the queries return null.

Now, I managed to achieve this by using the following calls:

var _session = // database session
A.CallTo(_session).WithReturnType<Foo>().Returns(null);
A.CallTo(_session).WithReturnType<Bar>().Returns(null);
// ~10 more of these with just different types defined

however I was wondering if there is a way of setting this up as the default behaviour - so that all calls to _session with a generic parameter will return null by default?

I don't know if it is possible in the first place, but it would probably be a nice feature.

@blairconrad
Copy link
Member

This issue came from StackOverflow question How to set the return value of multiple generic functions with FakeItEasy?.

@blairconrad
Copy link
Member

I couldn't figure out a way to do this. I think we'd need something like a WithNonVoidReturnType or WhenReturnTypeMatches addition to the A.CallTo(fake) use case.

@blairconrad
Copy link
Member

@balazsbonis, thanks for the idea. Assuming the other owners are also in favour of the addition, are you interested in working on it? The best course would be to discuss an API here, and if/once it's agreed on, to proceed with an implementation.

@thomaslevesque
Copy link
Member

I like WithNonVoidReturnType. It should be pretty easy to add:

IAnyCallConfigurationWithReturnTypeSpecified<object> WithNonVoidReturnType();

Of course, with this it becomes possible to return a value of the wrong type, so it will fail when the method is called...

@blairconrad
Copy link
Member

Oh, agreed. It's not typesafe. Just like many of the other more flexible/powerful methods.

@adamralph
Copy link
Contributor

What about:

A.CallTo(_session).ReturnsDefaultValue();

I.e. we just switch off the returning of automatic fakes for that object. We could also make it work with Where() for more control:

A.CallTo(_session).Where(call => ...).ReturnsDefaultValue();

@blairconrad
Copy link
Member

@adamralph, I don't think @thomaslevesque's suggestion precludes using Where, and it would allow (assuming an appropriate non-typed Returns and ReturnsLazily) the client to set up any return value. I think it's preferable to allow the client to pick the value, instead of just limiting ourselves to defaults.

@blairconrad
Copy link
Member

To expand, the way I thought @thomaslevesque's suggestion would work would be

A.CallTo(fake).Where().WithNonVoidReturnType().Returns(null);

(or swap Where and WithNonVoidReturnType, as desired, like we can now with Where and WithReturnType<>)

@adamralph
Copy link
Contributor

What would be the use case for a non-default value? Would it not be covered by WithReturnType<T>()?

@blairconrad
Copy link
Member

Someone may have a pre-existing factory method for making their preferred values?

@adamralph
Copy link
Contributor

After discussion with @blairconrad, I'm 👍 for WithNonVoidReturnType().

@balazsbonis
Copy link
Collaborator Author

@blairconrad I have never seen FakeItEasy's code yet, so I don't know how slow would it take me to actually implement the changes. Therefore if you already have a nice design of what exactly needs to be changed, I say you go ahead. I really like the idea you came up with. 👍

@adamralph
Copy link
Contributor

@balazsbonis speed isn't really important. This is a non-breaking change and it can be added at any time. If you'd like to take the time to learn the code base and make the change, we'd be more than happy to wait for it.

@blairconrad
Copy link
Member

@balazsbonis, @adamralph beat me to it. We're in no rush for the feature, and if it benefits you and you'd enjoy working on it, we'd love to have your submission. And we'd be happy to talk through any questions you'd have, or give any hints, if you're feeling stuck.

@blairconrad
Copy link
Member

Oh, and just so it doesn't look like I'm taking credit, @thomaslevesque came up with the syntax.
English's "you" is confusing, as you (see what I did there?) can't always tell whether it's singular or plural, so I wasn't sure whether credit was accruing to me or the team!

@thomaslevesque
Copy link
Member

Oh, and just so it doesn't look like I'm taking credit, @thomaslevesque came up with the syntax.

Actually, @blairconrad came up with the name 😉

I looked at the code, it should be pretty easy to implement. Great feature to discover the code base @balazsbonis 😄

@blairconrad
Copy link
Member

@thomaslevesque

Actually, @blairconrad came up with the name 😉

Oh. I guess so. I need my "embarrassed" emoji!

@adamralph
Copy link
Contributor

Marked as taken, since @balazsbonis has started work on this in #674.

@adamralph
Copy link
Contributor

Still needs docs.

@adamralph adamralph reopened this Jun 15, 2016
@blairconrad
Copy link
Member

And I wouldn't say "no" to a diagnostic that notices when we're not using the result.
This could be a separate issue or PR if we want.

@adamralph
Copy link
Contributor

Nice idea. I'd be happy with it being a separate issue. I guess the same applies to WithReturnType<TMember>()?

@blairconrad
Copy link
Member

the same applies to WithReturnType<TMember>()?

already covered

@adamralph
Copy link
Contributor

Oh! That easy! Well, let's just add it then.

But! Should it be a separate enhancement issue also labelled analyzer?

@adamralph
Copy link
Contributor

adamralph commented Jun 16, 2016

What shall we do about the analyzer? I don't think we need a separate issue, we can just say that this feature comes complete with docs, analyzer warning, etc.

@thomaslevesque
Copy link
Member

What shall we do about the analyzer? I don't think we need a separate issue, we can just say that this feature complete with docs, analyzer warning, etc.

Yes, I'll reopen and submit a PR with the analyzer changes.

@blairconrad
Copy link
Member

we can just say that this feature comes complete with docs, analyzer warning, etc.

That's my preference in all things. It's part of the reason that any time I get an opportunity, I say "let's move it into the repo" where "it" could be docs, release notes, or whatever.

@blairconrad
Copy link
Member

This issue has been fixed in release 2.1.0.

Thanks for your help, @balazsbonis. Look for your name in the release notes! 🏆

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

Successfully merging a pull request may close this issue.

4 participants