Re-introduce Microsoft.Extensions.ObjectPool.DependencyInjection #4038
Conversation
|
This is accounting for the aspnetcore api-review (method names, namespaces, api) |
|
Once this is merged please make an update in https://github.com/dotnet/api-catalog-infra/ - either update https://github.com/dotnet/api-catalog-infra/pull/59 (if it's still unmerged) or apply the renaming separately. |
...l.DependencyInjection.Tests/Microsoft.Extensions.ObjectPool.DependencyInjection.Tests.csproj
Show resolved
Hide resolved
src/Libraries/Microsoft.Extensions.ObjectPool.DependencyInjection/PoolOptions.cs
Outdated
Show resolved
Hide resolved
...Microsoft.Extensions.ObjectPool.DependencyInjection/ObjectPoolServiceCollectionExtensions.cs
Outdated
Show resolved
Hide resolved
...Microsoft.Extensions.ObjectPool.DependencyInjection/ObjectPoolServiceCollectionExtensions.cs
Show resolved
Hide resolved
| /// </summary> | ||
| [Experimental] | ||
| public static class ObjectPoolServiceCollectionExtensions | ||
| { |
There was a problem hiding this comment.
Can we get back the overload that consumes a configuration section? We want to systematically enable things to be configured in this way and it's the convention in this repo.
Can we call all the configuration delegate just "configure" which is the convention we use in this repo?
Thanks.
There was a problem hiding this comment.
Currently looking into why the section was removed after aspnetcore api-review, they had some arguments that might be worth checking.
configureOptions: there are a few occurrences in this repos in case you think it's worth going through the rest of the code for consistency.
There was a problem hiding this comment.
@geeknoid This is like a custom deserialization for these settings. I see that it's checking the value is a valid integer, what else is it providing over the standard configuration methods?
There was a problem hiding this comment.
So yes, we should change "configureOptions" to "configure" for general consistency.
Within this repo, we generally use a triplet of functions for configuring components. No options (you get the defaults), an Action delegate which lets you override options, and a ConfigurationSection to make it easy to consume the configuration state from an external JSON. This model has been working well for us.
@tekian Jan helped establish this pattern 3 years ago or so, maybe he has additional insights to provide.
There was a problem hiding this comment.
With respect to configuration section, we make no assumptions about where in the configuration hierarchy is the section to configure the component. We let developer decide and hand us the section to bind to. IConfigurationSection, as compared to IConfiguration, knows Path which was an information we wanted to capture to be able to reconstruct configuration schema.
There was a problem hiding this comment.
Thats fine but 90% of our code takes IConfiguration, not IConfigurationSection and it usually parses a known schema (more than just an int). The pattern should be consistent with the rest of the APIs in the stack.
...ies/Microsoft.Extensions.Http.Resilience/Hedging/Routing/RoutingStrategyBuilderExtensions.cs
Outdated
Show resolved
Hide resolved
...Microsoft.Extensions.ObjectPool.DependencyInjection/ObjectPoolServiceCollectionExtensions.cs
Outdated
Show resolved
Hide resolved
...Microsoft.Extensions.ObjectPool.DependencyInjection/ObjectPoolServiceCollectionExtensions.cs
Outdated
Show resolved
Hide resolved
# Conflicts: # src/ToBeMoved/DependencyInjection.Pools/DependencyInjectionExtensions.cs
| // The .NET Foundation licenses this file to you under the MIT license. | ||
|
|
||
| using System.Diagnostics.CodeAnalysis; | ||
|
|
||
| namespace Microsoft.Extensions.DependencyInjection.Pools; | ||
| namespace Microsoft.Extensions.ObjectPool; | ||
|
|
||
| /// <summary> | ||
| /// Contains configuration for pools. | ||
| /// </summary> | ||
| [Experimental] | ||
| public sealed class PoolOptions |
There was a problem hiding this comment.
Given that this is now in the general-purpose Microsoft.Extensions.ObjectPool namespace, perhaps we should give this a more specialized name? ServiceCollectionPoolOptions maybe? Any other ideas?
There was a problem hiding this comment.
This one is hard, DependencyInjectionPoolOptions? But the current name loks fine to me, there is no conflict and users will probably not use the class name directly.
There was a problem hiding this comment.
Why didn't this get called ObjectPoolOptions?
There was a problem hiding this comment.
I agree with Martin. It looks strange, that this type (or ObjectPoolOptions) is not used by Microsoft.Extensions.ObjectPool || Microsoft.Extensions.ObjectPool<T>.
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
@RussKie flaky tests? also the Tests tab is empty on azdo, the one listing all failed tests and artifacts. |
I believe this is caused by #4078. I have a hunch Microsoft.Extensions.Diagnostics.ResourceMonitoring.Test could be unsettling the build agent - I saw a few build runs where tests from that project failed. |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
@sebastienros please don't forget about #4038 (comment). |
ghost
locked as resolved and limited conversation to collaborators
null
Microsoft Reviewers: Open in CodeFlow