Use Microsoft.Extensions.Options to configure AutoMapper

[berhir]

This PR shows how the options pattern can be added in a non-breaking way.

See #132 for discussion.

The main difference is that AddAutoMapper can be called multiple times now. The first call registers the IConfigurationProvider and the IMapper. This means only the first call can set the service lifetime.

[jbogard]

I like it. I think we just need a test around this and we're good to go.

[lbargaoanu]

Maybe also a test that shows why the options stuff is useful, because I, for one, don't see it. It seems to me that you can implement it without it.

[jbogard]

To me this is useful if for nothing else than prevent errors, since in the past we've seen issues with folks double-registering etc.

[lbargaoanu]

The feature is AddAutoMapper per module, I get that. But a few things. Reusing the name for smth that works in such a different way seems prone to misunderstandings. This assumes services are registered by hand. I'm not sure that fits and also I don't think that's what people want/expect. And, implementation wise, I'm not sure what the options package reference brings if it's not really needed.

[jbogard]

I'm not sure this is terribly different than our current behavior, though. It does all the normal things it used to except it's additive without having to do some wonky global state.

[lbargaoanu]

You can call it multiple times and it doesn't scan.

[jbogard]

Where do you see that? I see it only skipping the IMapper and MapperConfiguration registration.

[lbargaoanu]

There is an overload that doesn't take assemblies/types. People are supposed to understand that if you don't pass anything, you must do things by hand. In previous versions, no assemblies meant all the ones already loaded. I don't know :)

[jbogard]

Oh, that behavior has been gone for a while. I killed it because the AppDomain stuff never seemed to work right. You HAVE to pass in something now (in fact, we should check for it).

[lbargaoanu]

That works now, you have to pass smth, but his PR explicitly allows not to pass anything, that's the point I guess. Also, if you do pass smth, you must be careful not to have overlaps. Not a big deal, this one, but a change from the current behavior.

[jbogard]

Ah, I see, the extra overload. Yeah, I'm OK with that. You might not want scanning, either, and this is a way around that.

[lbargaoanu]

Well, I think it's gonna be a problem :)

[berhir]

Reusing the name for smth that works in such a different way seems prone to misunderstandings.

I would not say that it works "in such a different way". If you upgrade the library to this version and you don't change anything, you will not even notice that something has changed. The existing methods behave the same when you use it the same way as before (calling it only once). You CAN call it multiple times if you want to have the configuration in different places, but you don't have to do it.

This assumes services are registered by hand. I'm not sure that fits and also I don't think that's what people want/expect.

This is just an additional option. You don't have to use it. You can use the library as before and it will behave the same as before. It's about giving people the choice if they don't want to use assembly scanning (like me). And there are other issues asking for this too: #127, #25, #116, #118

And, implementation wise, I'm not sure what the options package reference brings if it's not really needed.

Adding additional dependencies should be done with care, I agree. But the purpose of this library is to use AutoMapper with M.E.DI. This means in most cases this library is used with ASP.NET Core or the generic host. In both cases, the options package is used anyway because the framework uses it everywhere.

There is an overload that doesn't take assemblies/types. People are supposed to understand that if you don't pass anything, you must do things by hand.

Actually, you must pass at least a config action to the additional overload. You could pass an empty configuration, but it would be strange to expect that in this case there happens some magic to configure everything automatically. You can pass types/assemblies if you want to use scanning. If you don't pass it, there is nothing to scan and you must configure it manually. For me, it sounds not too complicated or unclear. Can you describe why you think it can be a problem? Maybe I miss something.

In the end, it's a decision you as the owners of the library have to make. If you agree that it's useful, I can add some tests for it.

[lbargaoanu]

@berhir @jbogard is fine with everything, so go right ahead :)

[jbogard]

I added a couple of tests here around double registration.

[berhir]

@jbogard thanks for adding the tests and merging the PR. I thought about more complex tests and was not able to finish it yet. My idea was to run most of the existing tests multiple times with different configurations.

• Configuration using assembly scanning
• Manual configuration
• A mix of assembly scanning and manual configuration

This would cover more cases but it would also require a lot of changes in the tests. If you think it's not necessary it's ok. otherwise, my colleague @ironcev and I can think about it and open another PR.

[github-actions[bot]]

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.