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
Reinstate LoggerSinkConfiguration.Sink(ILogEventSink, LogEventLevel)
#1889
Conversation
…to avoid binary compatibility problems
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.
LGTM
publicApi.ShouldMatchApproved(options => | ||
{ | ||
// Comment this line out to view the failure as a diff. Leave it here so that CI builds don't hang when this test fails. | ||
options.NoDiff(); |
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.
How does hang look like? I do not use NoDiff
in other repos and did not see issues.
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.
That's a good question, I may have jumped the gun here, based on experience with other tools that launch windowed executables for this.
/// <seealso cref="Sink(ILogEventSink, LogEventLevel, LoggingLevelSwitch?)"/> | ||
/// <returns>Configuration object allowing method chaining.</returns> | ||
/// <remarks>Sink configuration methods that specify <paramref name="restrictedToMinimumLevel"/> should also specify <see cref="LoggingLevelSwitch"/>.</remarks> | ||
[EditorBrowsable(EditorBrowsableState.Never)] |
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.
Why not obsolete?
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.
It's impossible to stop the compiler selecting it when doing overload resolution, and thus marking it obsolete results in a spurious warning.
I just switched Seq over to Serilog 3 dev and the latest versions of our dependencies, but because of some transitive dependencies in the build I hit:
We speculatively removed it in #1800.
I think this is going to be a common experience; for the cost of one overload that wasn't already
[Obsolete]
I think walking this change back is a good trade-off.I've left the other removal from #1800 alone as we don't have any evidence of breakage at this point.