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.

Sign up for GitHub

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

Jump to bottom

Configure using the options pattern #483

Open
jeffdahl opened this issue Jan 17, 2023 · 6 comments · May be fixed by #540
Open

Configure using the options pattern #483

jeffdahl opened this issue Jan 17, 2023 · 6 comments · May be fixed by #540
Labels
enhancement New feature or request

Comments

@jeffdahl
Copy link

The options pattern simplifies configuration. When using a settings.json file, instead of configuring GoogleAds with the following three lines:

IConfigurationSection section = Configuration.GetSection("GoogleAdsApi");
GoogleAdsConfig config = new GoogleAdsConfig(section);
GoogleAdsClient client = new GoogleAdsClient(config);

configuration can be done in a single line:

using IHost host = Host.CreateDefaultBuilder(args)
    .ConfigureServices((hostContext, services) =>
        services.AddGoogleAdsClient(hostContext.Configuration))
    .Build();
@jeffdahl jeffdahl added the enhancement New feature or request label Jan 17, 2023
@jeffdahl jeffdahl mentioned this issue Jan 18, 2023
Closed
@AnashOommen
Copy link
Member

None of GoogleAdsConfig, GoogleAdsClient and services are designed to be singletons, because that's not how the product works in the general case.

I'll give it some thought, but perhaps it is simpler to have a SingletonGoogleAdsClientProvider helper class specifically for this use case. @Raibaz wdyt?

@jeffdahl
Copy link
Author

jeffdahl commented Jan 18, 2023 via email

@Raibaz
Copy link
Contributor

Raibaz commented Jan 24, 2023

Not entirely sure if registering GoogleAdsConfig and GoogleAdsClient as transient would be feasible; I'd rather keep them as they are and instead add a Singleton to wrap them for dependency injection usage.

@AnashOommen
Copy link
Member

@jeffdahl I think @Raibaz and I need some more time thinking through this. How about we accept this bug for ourselves, and send you a PR when we are ready? I'd be happy to credit you for the work even if we end up with a different implementation.

@jeffdahl
Copy link
Author

jeffdahl commented Feb 7, 2023 via email

@AnashOommen AnashOommen mentioned this issue Apr 4, 2023
Closed
@jeffdahl jeffdahl linked a pull request Oct 24, 2023 that will close this issue
Open
@jeffdahl
Copy link
Author

Any consideration of adding the two interfaces in PR #540? As explained in the PR, adding these two interfaces will allow a configured IGoogleAdsClient to be injected via the DI container.

Thanks for considering.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
enhancement New feature or request
Projects
None yet
Milestone
No milestone
3 participants
@Raibaz @AnashOommen @jeffdahl