I added2f2796b following thesuggestion from@leonardochaia

Awesome! It looks like I would be able to remove my DI workaround with this changes.

This allows to override injected dependencies for testing purposes!

Can we please add this last test case? :)

varparser=newCommandLineBuilder(command).ConfigureBindingContext(context=>context.AddService<IGreeter,GreeterImpl>()).ConfigureBindingContext(context=>context.AddService<IGreeter>(sp=>newGreeterMock()))// would use Moq or something here.Build();

IGreeter should be of typeGreeterMock
I think this should work since the IServiceProvider impl has a Dictionary with a Type for the key as far as I remember.

@leonardochaia I don't understand what you mean? The first one will be done by this PR, the second one you could always do already?

Sorry, should have been clearer.

I'm just trying to be sure that the secondConfigureBindingContext will override the first one (and don't throw, or silently ignore it)

That's why I would assert that

IGreeter should be of type GreeterMock

The second one thows??? Really? What exception do you get? 🤔

I'm sorry (again). Didn't mean to say it is throwing right now. My point is that, if the expected API is that multiple calls toConfigureBindingContext should be supported, and the latter call should be able to override things configured on previous calls, we should have a test that makes sure that that behavior is not gonna be broken in the future by someone i.e changing the inner code to throw when a service is already registered or something like that.

So that's why I propose we add a test that represents the actual use case of overriding an already registered service.

Something likeRegistered_services_can_be_overriden_by_subsequent_calls_to_ConfigurreBindingContext

EDIT: I'm not saying we should test theAddService methods work. We know they do by the previous tests as you mentioned. I'm interested in testing that subsequent calls toConfigureBindingContext will allow to override services registered on earlier calls, since that's the API I think we need for testing purposes

This looks awesome! 🎉

The newAddService overloads increase the scope of the DI features in ways that might be misleading and will likely lead to additional issues being opened when people misunderstand the goal. The pre-existingAddService method accepted aFunc so that you can know when it's called and control things like lifetime management, singleton versus transient instances, and recursive resolution, none of which System.CommandLine's service provider currently does for external types.

@jonsequitur

The new AddService overloads increase the scope of the DI features in ways that might be misleading and will likely lead to additional issues being opened when people misunderstand the goal.

Yeah, I can see that. Basically, all service registrations are technically Transient services (they all have a factory registration that is called each time the service is resolved), but of course the factory for the basic services returns singleton instances. However, the no-func overloads that I added really are Transient. Maybe add theTransient suffix? Would that help?

I'd prefer to avoid increasing the scope of the existing DI implementation at all, which originally lacked a publicAddService method of any kind.

A safer approach might be to allow people to bring their ownIServiceProvider implementation and therefore be able to completely control object lifetimes.

@jonsequitur Hmm, okay, so we could just remove all the overloads toAddService. So, what aboutCommandHandler.CreateLazyFromModelBinder? If I get a ModelBinder that would be able to create an instance regardless of whether it was added to the BindingContext, no? So we don't actually need to add Handlers as BindingContext services? (Provided paramless-ctor)

The way we create command handlers is undergoing some rethinking so that we can eventually remove all of the name-based matching and a lot of model binding complexity. If you're interested in the source generator approach we're looking at, take a look at this PR:#1362.