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

Implemented a ClientFactory to obtain a new instance of a ChatGptClient #45

Merged

Conversation

nicolaparo
Copy link
Contributor

Adding the AddChatGptClientFactory to add the ClientFactory.

Remarks:

  • I had to change the visibility of ChatGptServiceCollectionExtensions.ConfigureHttpClient from private to internal in order to be able to correctly configure the httpclient in the factory CreateClient method.
  • I changed also ChatGptOptions from class to record in order to support the defaultOptions cloning using with { }. This way the changes to the options are applied to a separate copy of the object in the CreateClient method.

this.defaultOptions = defaultOptions;
}

public IChatGptClient CreateClient(Action<ChatGptOptions> setupAction)
Copy link
Owner

Choose a reason for hiding this comment

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

The parameter should be nullable and optional, if I want to create a client with the default options and I don't need to change anything else. Moreover, an overload the accepts a ServiceProvider as argument is needed, to cover a scenario like this one:

public static IServiceCollection AddChatGpt(this IServiceCollection services, Action<IServiceProvider, ChatGptOptions> setupAction)

It is necessary when I need to provide options that have external dependencies (for example, an Api Key that depends on the current user).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Implemented the CreateClient method with 3 overloads: a parameterless version that uses the default options, plus two overloads that accept an Action<ChatGptOptions> or Action<IServiceProvider, ChatGptOptions>

setupAction.Invoke(options);

services.AddMemoryCache();
services.AddSingleton<IChatGptClientFactory, ChatGptClientFactory>(
Copy link
Owner

Choose a reason for hiding this comment

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

Why are you using this method and not simply, after adding ChatGptOptions itself to the service collection, like in

):

services.AddSingleton<IChatGptClientFactory, ChatGptClientFactory>();

/// <param name="services">The <see cref="IServiceCollection"/> to add services to.</param>
/// <param name="setupAction">The <see cref="Action{ChatGptOptions}"/> to configure the provided <see cref="ChatGptOptions"/>.</param>
/// <returns>A reference to this instance after the operation has completed.</returns>
public static IServiceCollection AddChatGptClientFactory(this IServiceCollection services, Action<ChatGptOptions> setupAction)
Copy link
Owner

Choose a reason for hiding this comment

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

If I'm creating a factory, there is the case in which I don't want to directly provide an Action to setup ChatGtpOptions, so this parameter should be nullable and optional.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added now an AddChatGptClientFactory(this IServiceCollection services) that allows to customize later the client

@marcominerva
Copy link
Owner

@nicolaparo, sorry for the long delay. Do you mind updating your PR to the latest commit of the library and then requesting a review? Thanks

@marcominerva marcominerva merged commit c853466 into marcominerva:develop Feb 12, 2024
3 checks passed
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.

None yet

2 participants