-
Notifications
You must be signed in to change notification settings - Fork 29
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
Simplify the constructors for PeriodicBatchingSink and BoundedConcurrentQueue. #30
Conversation
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.
Hi! Thanks for the PR. The BoundedConcurrentQueue
changes look good. We can't change the signature of the protected PeriodicBatchingSink
constructor, though, as existing assemblies dependent on it will fail to bind without a rebuild.
src/Serilog.Sinks.PeriodicBatching/Sinks/PeriodicBatching/PeriodicBatchingSink.cs
Outdated
Show resolved
Hide resolved
How about the new update, where the original constructors are maintained but a new one added. This has the added benefit that a call to the original constructor specifying the queueLimit now doesn't create a queue with no limit, then immediately replace it with one that has a limit. |
Hi Oliver - sorry about the delay! I'll take a look at this ASAP 👍 |
Okay, back in action! I think the implementation of the I am mildly unsure of the three constructors now being exposed to subclasses, especially the two three-argument versions where the only difference is in the nullability of the third I still lean toward making the change, but is there another way we can clean up the constructor list that I'm not considering, @serilog/reviewers-core? I.e. are there more arguments to |
The current way that the constructors are implemented prevents some types of configuration and sub-classes of
PeriodicBatchingSink
have two paths of constructor logic. This PR aims to simplify and improve this.The motivation was needing to extend PeriodicBatchingSink and pass in all configuration as follows:
Additionally I've added a RootNamespace declaration to fix warnings about incorrect namespaces.