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

expose container build to params #20

Merged
merged 7 commits into from
Oct 23, 2018
Merged

expose container build to params #20

merged 7 commits into from
Oct 23, 2018

Conversation

tg123
Copy link
Contributor

@tg123 tg123 commented Oct 10, 2018

for some cases, we had already created a builder and registered some components.
this is to reuse the container builder outside the AutoMock

@tillig
Copy link
Member

tillig commented Oct 10, 2018

For all of these method overloads you'll need to provide actual separate overloads instead of just optional parameters. The optional parameters actually change the interface (even though they're optional) and cause this to be a breaking change.

(I've hit this through experience, unfortunately.)

@tg123
Copy link
Contributor Author

tg123 commented Oct 11, 2018

Seems build need some fix

@tillig
Copy link
Member

tillig commented Oct 16, 2018

Yeah, I don't know who set that Travis CI build up but it hasn't worked in forever. The challenges with drive-by contributions and not enough people to own these extension libraries.

@tillig
Copy link
Member

tillig commented Oct 16, 2018

Before I pull this in: It occurs to me that while you may have registered some things in a container builder prior to setting up mocks, you can only build the container from a builder once - and this happens as soon as the AutoMock is created. Do you think it'll be a problem that people might not realize that as soon as you say, AutoMock.GetLoose(builder); that it instantly invalidates the builder so you can't add any more to it? It seems like it could be an easy way for someone else to shoot themselves in the foot and may create some support burden in the form of questions that need to be answered or documentation to write and maintain.

var builder = new ContainerBuilder();
builder.RegisterType<SomethingToMock>();

// This runs builder.Build()
var mock = AutoMock.GetLoose(builder);

// This will have no effect
builder.RegisterType<AnotherToMock>();

// This will throw an exception
var container = builder.Build();

Would a better API be something where you pass around an Action<ContainerBuilder> instead?

var myRegistrations = (ContainerBuilder builder) =>
{
  builder.RegisterType<SomethingToMock>();
  builder.RegisterType<AnotherToMock>();
};

var mock = AutoMock.GetLoose(myRegistrations);

This may require you to refactor your code so you create an Action<ContainerBuilder> instead of a ContainerBuilder but it'd also be more explicit about who owns the actual ContainerBuilder and would offer less opportunity for folks to accidentally fail.

What do you think?

@tg123
Copy link
Contributor Author

tg123 commented Oct 16, 2018

my approach, not added to this PR, is AutoMock.GetLoose(builder, callbackBeforeCallBuild)

I think it is enough to just expose builder, at least for our case. Removing the callback for PR is that I think it is a bit overdesigned.

AutoFac is a fantastic library for IoC on .NET, really happy to send some PR.

@tillig
Copy link
Member

tillig commented Oct 16, 2018

Sure, I don't think there needs to be a callback parameter and a builder, I'm thinking maybe only the callback, and likely have that run before the other registrations that AutoMock does. What I'm thinking about is that folks using this library could easily get confused with the new API. "Is the AutoMock.GetLoose(builder) adding things into the container? Why isn't the stuff I added after the GetLoose call correctly mocked? How come I can't just pass in a container?"

I'm thinking not only of the intent of your current PR, but how do we make sure the API is something we can take forward and support. How many questions will we get with people not intuitively understanding what to do here?

Folks don't always read the docs or look for examples, sometimes they just plug things in and assume it'll work the way they guess it will. How do we make it so they'll guess right?

I'm also thinking more about this - if it's supposed to automatically mock but you're partially building a container already it makes me wonder if you're possibly not using this the way it's intended. If you are already partially building a container, it makes more sense to just finish putting things in the container as needed and not use AutoMock stuff at all. If some things in here are "real" and some aren't, then it's not all auto mocks.

But I digress, if folks think this is interesting, that's fine. I don't auto mock anything so I don't have much use case here. I do end up having to answer questions and support things I don't contribute or have expertise in (like this) so making sure the API is super easy for people to use and not create challenging situations is important.

@tg123
Copy link
Contributor Author

tg123 commented Oct 18, 2018

I fix the test and changed to callback style (my first impl in our case) :)

{
this.MockRepository = repository;
var builder = new ContainerBuilder();
builder.RegisterInstance(this.MockRepository);
builder.RegisterSource(new AnyConcreteTypeNotAlreadyRegisteredSource());
builder.RegisterSource(new MoqRegistrationHandler(_createdServiceTypes));
beforeBuild?.Invoke(builder);
Copy link
Member

Choose a reason for hiding this comment

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

After I take this pull, I will probably put this after the RegisterInstance but before the RegisterSource calls to help alleviate issues like autofac/Autofac#341.

@tillig
Copy link
Member

tillig commented Oct 23, 2018

This looks awesome! Also, thanks for fixing the Travis CI build. Much appreciated!

@tillig tillig merged commit 8ca388f into autofac:develop Oct 23, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants