Skip to content
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

User-configurable synchronized writes #66

Merged
merged 8 commits into from
Nov 22, 2019

Conversation

adamchester
Copy link
Member

@adamchester adamchester commented Aug 2, 2019

What issue does this PR address?
#65

Does this PR introduce a breaking change?
Yes, it's a breaking change. We could make it non-breaking (and I did originally), but it's feels like it might not be worth it. Adding overloads with a new required parameter syncRoot just for this seems a little too much.

Please check if the PR fulfills these requirements

  • The commit follows our guidelines
  • Unit Tests for the changes have been added (for bug fixes / features)

Wit the sample you can:

dotnet run -- --sync-root-separate which will produce garbled output:
image

dotnet run -- --sync-root-same which produce properly sync'd output:
image

dotnet run -- --sync-root-default which will not configure the syncRoot (leaving the default as per #65:
image

nblumhardt and others added 2 commits August 1, 2019 08:41
enabling scenarios where the application needs to sync with the
console, or the application wants to keep multiple console sinks which
do not need to be sync'd.
Copy link
Member

@nblumhardt nblumhardt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should bump the major version for this, although the (binary API) breakage will be minimal.

Before merging (with the bumped major) we also should make sure any minor revision changes sitting on dev are merged to master and released.

sample/ConsoleDemo/Program.cs Outdated Show resolved Hide resolved
Copy link
Member

@nblumhardt nblumhardt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great! I think this is good to go. I'll check on the state of dev and increment the major version number before merging.

@nblumhardt nblumhardt merged commit e83f2f3 into serilog:dev Nov 22, 2019
@adamchester adamchester deleted the configurable-locking branch November 23, 2019 00:38
@adamchester
Copy link
Member Author

🎉

@GitSnafu
Copy link

This is great, exactly what I was looking for. Any idea when this might be merged to master?

@nblumhardt
Copy link
Member

#82 is tracking this

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants