Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Re-introduce Microsoft.Extensions.ObjectPool.DependencyInjection #4038
Re-introduce Microsoft.Extensions.ObjectPool.DependencyInjection #4038
Changes from all commits
502d135
b92b748
e58470c
13bc614
001f276
c94445b
1a9cd85
42a45a5
09caa86
19811b9
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 toIConfiguration
, knowsPath
which was an information we wanted to capture to be able to reconstruct configuration schema.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.