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

Limit typeparams for Fake options and builder classes #1664

Merged
merged 1 commit into from
Oct 23, 2019

Conversation

blairconrad
Copy link
Member

@blairconrad blairconrad commented Oct 16, 2019

Could've been done as part of #1465, but I didn't think of it.
In addition to just providing a better view for the public API, this change removes 6 nullability warnings:

Creation\FakeOptions.cs(20,68): warning CS8604: Possible null reference argument for parameter 'obj' in 'void Action<object>.Invoke(object obj)'. 
Creation\FakeOptions.cs(84,31): warning CS8604: Possible null reference argument for parameter 'argument' in 'void Guard.AgainstNull(object argument, string argumentName)'. 
Creation\FakeOptions.cs(85,60): warning CS8604: Possible null reference argument for parameter 'fakedObject' in 'void FakeOptions<T>.ConfigureFakeToWrap(object fakedObject, object wrappedObject)'. 
Creation\FakeOptions.cs(85,66): warning CS8604: Possible null reference argument for parameter 'wrappedObject' in 'void FakeOptions<T>.ConfigureFakeToWrap(object fakedObject, object wrappedObject)'. 
FakeOptionsExtensions.cs(24,51): warning CS8604: Possible null reference argument for parameter 'fakedObject' in 'FakeManager Fake.GetFakeManager(object fakedObject)'. 
FakeOptionsExtensions.cs(56,59): warning CS8604: Possible null reference argument for parameter 'fake' in 'IAnyCallConfigurationWithNoReturnTypeSpecified A.CallTo(object fake)'. 

thomaslevesque
thomaslevesque previously approved these changes Oct 17, 2019
Copy link
Member

@thomaslevesque thomaslevesque left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @blairconrad, looks good to me. Why [WIP]?

@blairconrad
Copy link
Member Author

It's technically breaking. Check out the labels. We made the same distinction with #1465.

@thomaslevesque
Copy link
Member

Ah, yes, of course. So... Just keep this open until we decide to release 6.0?

@blairconrad
Copy link
Member Author

Just keep this open until we decide to release 6.0?

Pretty annoying, right? On the other hand, how different is it from keeping #1641 open until then?

We can close the PR, if you like. Or open an issue referencing the PR, then close the PR and reopen later!

I'm secretly hoping we'll move onto 6.0 soonish. I'm nearly through my "reduce nullability warning without making user-visible changes" work.

@thomaslevesque
Copy link
Member

On the other hand, how different is it from keeping #1641 open until then?

It's not, I guess!

We can close the PR, if you like. Or open an issue referencing the PR, then close the PR and reopen later!

No, it's fine!

I'm secretly hoping we'll move onto 6.0 soonish. I'm nearly through my "reduce nullability warning without making user-visible changes" work.

Nice!
We have a few on-hold issues, I think.
Time to think of other urgent breaking changes!

@thomaslevesque
Copy link
Member

Looks like your branch is still out of date... Did you rebase after merging #1672 ?

@blairconrad
Copy link
Member Author

Oh, sorry. I didn't mean to annoy you with this. I knew you had other commits in the hopper, and probably shouldn't've pushed. Will hold off repushing.

@blairconrad blairconrad changed the title [WIP] Limit typeparams for Fake options and builder classes Limit typeparams for Fake options and builder classes Oct 23, 2019
@thomaslevesque
Copy link
Member

Oh, sorry. I didn't mean to annoy you with this. I knew you had other commits in the hopper, and probably shouldn't've pushed. Will hold off repushing.

Oh, I don't mind. It shouldn't conflict with the other PR anyway

@blairconrad
Copy link
Member Author

Properly rebased now, @thomaslevesque! No rush.

Copy link
Member

@thomaslevesque thomaslevesque left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still looking good! Thanks @blairconrad

@thomaslevesque thomaslevesque merged commit 8158ea0 into FakeItEasy:master Oct 23, 2019
@blairconrad blairconrad deleted the constrain-class branch October 23, 2019 22:12
@blairconrad
Copy link
Member Author

Thank you,, sir.

@blairconrad blairconrad added this to the vNext milestone Oct 23, 2019
@afakebot
Copy link

This change has been released as part of FakeItEasy 6.0.0-beta.1.

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

Successfully merging this pull request may close these issues.

None yet

3 participants