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

Make all default return values Dummies, even after Invokes or DoesNothing #830

Closed
blairconrad opened this issue Aug 17, 2016 · 19 comments
Closed
Assignees
Milestone

Comments

@blairconrad
Copy link
Member

@blairconrad blairconrad commented Aug 17, 2016

Spun off from #806.

Currently, an unconfigured method with a return value will return a Dummy of the appropriate type when called.

However, as soon as we start building a rule (for example, when we add an Invokes behaviour to the method), we create a rule that will return the "default value" for the type, as we can see in BuildableCallRule's constructor. GetDefaultValue returns null for a reference type or an instance for a value type.

And then if we decide to have the method DoesNothing, the rule is modified so that it will not set the return value at all (and this is the cause of the bug that @thomaslevesque found).

I think that all three situations should use the "unconfigured behaviour" and return a Dummy (or nothing for a void of course).

@blairconrad
Copy link
Member Author

@blairconrad blairconrad commented Aug 17, 2016

I struggled with the bug/enhancement labels for this one. In the end, I figured if we thought that #806 was a bug, because users are forced to specify a return value or get undesired behaviour, then so too is this a bug. I could be persuaded.

@adamralph
Copy link
Contributor

@adamralph adamralph commented Aug 17, 2016

Marking as on-hold until we decide we want to go ahead with 3.0.

@blairconrad
Copy link
Member Author

@blairconrad blairconrad commented Aug 17, 2016

Ah. I should've thought of that. Then again, wasn't entirely sure that everyone was on board.
In particular, thought on-hold only applied when ready.

@adamralph
Copy link
Contributor

@adamralph adamralph commented Aug 17, 2016

thought on-hold only applied when ready

That probably makes sense, although I'm not sure we're all on board with only bringing things into in-progress which are first ready, so it may better to apply on-hold earlier rather than later.

@thomaslevesque
Copy link
Member

@thomaslevesque thomaslevesque commented Sep 12, 2016

I hadn't realized this issue was still open. I agree with the proposed behavior.

@blairconrad
Copy link
Member Author

@blairconrad blairconrad commented Sep 12, 2016

Thanks, @thomaslevesque. Open because breaking. Otherwise, I'd've been on it like Garfield on lasagna.

@adamralph
Copy link
Contributor

@adamralph adamralph commented Nov 15, 2016

Is this something we want to do in 3.0?

@blairconrad
Copy link
Member Author

@blairconrad blairconrad commented Nov 15, 2016

It's something I'd like to do.
Unless we're in a tearing rush for 3.0, I'd try to get it in.

@blairconrad
Copy link
Member Author

@blairconrad blairconrad commented Nov 15, 2016

It's something I'd like to do.

by which I mean I think we should do it. Not claiming. Although I would do it.

@thomaslevesque
Copy link
Member

@thomaslevesque thomaslevesque commented Nov 15, 2016

I also think we should do it 👍

@adamralph
Copy link
Contributor

@adamralph adamralph commented Nov 15, 2016

Fine by me. Adding to 3.0.0 milestone and un-on-holding.

@blairconrad
Copy link
Member Author

@blairconrad blairconrad commented Nov 20, 2016

Thanks for the merge!
Oh. I should update release notes. I wonder if we're going for an alpha002 or a beta001…

@thomaslevesque
Copy link
Member

@thomaslevesque thomaslevesque commented Nov 20, 2016

I wonder if we're going for an alpha002 or a beta001…

Well, Castle.Core 4.0.0 is beta now, so I guess we could do a beta as well, and release a final version once Castle.Core goes final too.

@adamralph
Copy link
Contributor

@adamralph adamralph commented Nov 20, 2016

Beta generally means that no more breaking changes are planned. Given that we still have #910 on the 3.0.0 milestone, I'm not sure we can progress past alpha yet.

I'm also not sure about #904 since I haven't had a chance to catch up on the conversation there yet.

@blairconrad
Copy link
Member Author

@blairconrad blairconrad commented Nov 20, 2016

Beta generally means that no more breaking changes are planned.

Whoa. Did we not meet that goal in the lead-up to 2.0!

I guess maybe at each release we hadn't planned any breaking changes.

@adamralph
Copy link
Contributor

@adamralph adamralph commented Nov 20, 2016

Yeah, I think the transition from alpha to beta is still at the mercy of breaking changes that you don't yet know you need to do, but I think that's fine.

@thomaslevesque
Copy link
Member

@thomaslevesque thomaslevesque commented Nov 20, 2016

Beta generally means that no more breaking changes are planned

I thought that was RC?

@adamralph
Copy link
Contributor

@adamralph adamralph commented Nov 20, 2016

An RC is the version that will be RTM, unless someone finds a bug. I.e. RTM should ideally be built off the same commit. If you look at our current 3.0.0 milestone, after we close #910, the remaining work is deprecating self initialised fakes and adding acceptance tests for netstd, which sound like text book "remaining things to do after the beta release", before RC.

@blairconrad
Copy link
Member Author

@blairconrad blairconrad commented Feb 22, 2017

This issue has been released in FakeItEasy 3.0.0:
https://github.com/FakeItEasy/FakeItEasy/releases/3.0.0

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

Successfully merging a pull request may close this issue.

None yet
3 participants
You can’t perform that action at this time.