feat: simplify logging configuration #288
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
cprice404
force-pushed
the
implement-retries
branch
from
3ef2590
to
efa1c92
Compare
kvcache
previously approved these changes
Oct 7, 2022
There was a problem hiding this comment.
this looks great to me. I think IMiddleware is significantly better this way, and it's more clear to me now how to configure logging. The burden of invoking a static function instead of invoking a property to me seems very light and a welcome price to pay for the clarity.
malandis
previously approved these changes
Oct 7, 2022
Kenny called out on a previous PR that there was some code smell in the way I was managing the logging factories for the various components of the configuration. This commit reworks things slightly so that the user still only has to pass in the ILoggerFactory in one spot in our pre-built configs, but we now just pass that directly through to the constructors of the other things that need it. This allows us to get rid of the getters and copy constructors that we had previously exposed.
cprice404
force-pushed
the
clean-up-logging
branch
from
2ab51c9
to
ca6c5f4
Compare
|
rebased |
eaddingtonwhite
approved these changes
Oct 10, 2022
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Kenny called out on a previous PR that there was some code smell in the way I was managing the logging factories for the various components of the configuration.
This commit reworks things slightly so that the user still only has to pass in the ILoggerFactory in one spot in our pre-built configs, but we now just pass that directly through to the constructors of the other things that need it. This allows us to get rid of the getters and copy constructors that we had previously exposed.