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

Combine StubbedProperty{G|S}etterSetup #1204

Merged
merged 1 commit into from
Aug 24, 2021

Conversation

stakx
Copy link
Contributor

@stakx stakx commented Aug 24, 2021

This is a follow-up to #1203.

@stakx stakx force-pushed the refactor/stubbed-property-setup branch from f2c59bf to 84bf35a Compare August 24, 2021 15:36
Comment on lines -206 to -211
if (ProxyFactory.Instance.IsMethodVisible(getter, out _))
{
propertyValue = CreateInitialPropertyValue(mock, getter);
getterSetup = new StubbedPropertyGetterSetup(mock, expression, getter, () => propertyValue);
mock.MutableSetups.Add(getterSetup);
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Those visibility checks are no longer required because we already know that either the getter or the setter can be intercepted by the proxy factory; otherwise we wouldn't end up here in the first place.

If the other accessor isn't interceptable, that amounts to the same thing as if it were completely absent: we'll still create a setup just for the present accessor (even if it won't be of much actual use beyond preventing this code from executing repeatedly for the same property).

Comment on lines +637 to +645
if (!pi.CanRead(out var getter))
{
Guard.CanRead(pi);
}

if (!pi.CanWrite(out var setter))
{
Guard.CanWrite(pi);
}
Copy link
Contributor Author

@stakx stakx Aug 24, 2021

Choose a reason for hiding this comment

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

This isn't pretty. Seems like we're missing a method that acts as both guard and for accessor(s) retrieval (or perhaps even better, an exception factory method).

@@ -82,7 +82,7 @@ public void RemoveAllPropertyAccessorSetups()

lock (this.setups)
{
this.setups.RemoveAll(s => s is MethodSetup ms && ms.Method.IsPropertyAccessor());
this.setups.RemoveAll(s => s is StubbedPropertySetup || (s is MethodSetup ms && ms.Method.IsPropertyAccessor()));
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This line of code is getting really bad. It's even more important now to get rid of the whole method, if possible; see #1203 (comment).

src/Moq/StubbedPropertySetup.cs Outdated Show resolved Hide resolved
@stakx stakx force-pushed the refactor/stubbed-property-setup branch from 84bf35a to c0e1256 Compare August 24, 2021 15:39
@stakx stakx merged commit 7848788 into devlooped:main Aug 24, 2021
@stakx stakx deleted the refactor/stubbed-property-setup branch August 24, 2021 15:57
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

1 participant