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

Swap order of IServiceProvider/IChatClient to ChatClientBuilder delegate? #5660

Closed
stephentoub opened this issue Nov 18, 2024 · 1 comment · Fixed by #5664
Closed

Swap order of IServiceProvider/IChatClient to ChatClientBuilder delegate? #5660

stephentoub opened this issue Nov 18, 2024 · 1 comment · Fixed by #5664

Comments

@stephentoub
Copy link
Member

ChatClientBuilder now has the following overloads:

public ChatClientBuilder Use(Func<IChatClient, IChatClient> clientFactory)
public ChatClientBuilder Use(Func<IServiceProvider, IChatClient, IChatClient> clientFactory)

Shouldn't the order of IServiceProvider and IChatClient be swapped in the second one? Effectively the IServiceProvider is an optional argument to the factory delegate, and such arguments typically come later than the required ones. I realize there are other components in the DI ecosystem that accept the args in this order, but that feels wrong to me, too.

Same for the embedding generator.

cc: @SteveSandersonMS

@SteveSandersonMS
Copy link
Member

That seems like a reasonable change to me.

Having services go first was a continuation of DI patterns we have elsewhere. But I agree that it's odd in this case and would be more satisfactory to have the optional param last.

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

Successfully merging a pull request may close this issue.

2 participants