-
Notifications
You must be signed in to change notification settings - Fork 126
FileLifecycleHooks
#80
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
Changes from 1 commit
f7bfe4e
9eee731
83d817f
63c3601
fca6eb9
0c65484
e604418
873f6d4
e310ab9
1864db1
b660b51
b904968
b6090cc
34344ad
0ae234e
55fcb2b
d4fa80a
9f7352d
ca0ac8c
5b3d64a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,17 @@ | ||
using System.IO; | ||
|
||
namespace Serilog | ||
cocowalla marked this conversation as resolved.
Show resolved
Hide resolved
|
||
{ | ||
/// <summary> | ||
/// Wraps the log file's output stream in another stream, such as a GZipStream | ||
/// </summary> | ||
public abstract class StreamWrapper | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Since the parameter lists to methods of are hot real-estate :-) we might avoid some future churn by naming this There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm with you on renaming At first I wasn't too sure about
The idea would be that if you're not interested in changing the stream, you just return |
||
{ | ||
/// <summary> | ||
/// Wraps <paramref name="sourceStream"/> in another stream, such as a GZipStream, then returns the wrapped stream | ||
/// </summary> | ||
/// <param name="sourceStream">The source log file stream</param> | ||
/// <returns>The wrapped stream</returns> | ||
public abstract Stream Wrap(Stream sourceStream); | ||
cocowalla marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,25 @@ | ||
using System.IO; | ||
using System.IO.Compression; | ||
|
||
namespace Serilog.Sinks.File.Tests.Support | ||
{ | ||
/// <inheritdoc /> | ||
/// <summary> | ||
/// Demonstrates the use of <seealso cref="T:Serilog.StreamWrapper" />, by compressing log output using GZip | ||
/// </summary> | ||
public class GZipStreamWrapper : StreamWrapper | ||
{ | ||
readonly int _bufferSize; | ||
|
||
public GZipStreamWrapper(int bufferSize = 1024 * 32) | ||
{ | ||
_bufferSize = bufferSize; | ||
} | ||
|
||
public override Stream Wrap(Stream sourceStream) | ||
{ | ||
var compressStream = new GZipStream(sourceStream, CompressionMode.Compress); | ||
return new BufferedStream(compressStream, _bufferSize); | ||
} | ||
} | ||
} |
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.
The added parameter here will be a binary breaking change, even though calling code will recompile successfully.
There are a couple of options - for
FileSink()
, we should probably add a constructor overload to accept the new parameter, and have the old calls redirect to it.At the level of these extension methods, it's possible to add an overload also, but the old methods have to be carefully un-defaulted and
[Obsolete]
d so that recompilation targets the new method (thereby giving us some future room to remove the old code). There are some examples of this in this file - see the first two methods in this class. Probably the best option for now, though in some future5.0
we might start cleaning this up.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.
Regarding
FileSink
, I just wanted to be clear that this is what you mean:After doing that, existing calls such as
new FileSink(path, new JsonFormatter(), null)
result in a compiler error due toThe call is ambiguous
. I can of course change all the calls in the library so they use the new ctor, but I guess this ctor ambiguity will break things for users?The extension methods are rather fiddly to get right :nerd: but I'll see what I can do!
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.
Usually in the past we've resolved the ambiguity by removing the
= x
default parameter values from the older version of the constructor. Alternatively, the new parameter can be positioned before the first defaulted one and not given a default value.Uh oh!
There was an error while loading. Please reload this page.
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.
If adding a new defaulted parameter breaks binary compatibility, I would have though that removing defaults would have too?
2nd option would defo fix it though
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.
From the loader's perspective, only the number/kind/order of parameters is important, it doesn't see the default values at all. So adding a parameter, even if it has a default, changes the signature. Adding/removing/modifying default values doesn't break binary compatibility because they're not considered part of the signature. Fun and games :)
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.
Hmm, OK, interesting 😄
So let's say that a user calls this today:
new FileSink(path, textFormatter, 12345);
And then we remove the defaults from the remaining
encoding
andbuffered
parameters, and do this:In this scenario, will the old code continue to work?
I feel there must be some tooling that can check if binary compatibility is maintained - do you use anything like that?
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.
Yes, the old code will be binary compatible here, because the compiler inserts the default values directly into the call site.
So far I haven't found/used any recent tooling for this, but it could be out there. Mostly just painstaking work :-)
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.
https://www.fuget.org/ can be used to compare the API changes between 2 published versions of the same Nuget package. Quite helpful before remove the "pre-release" suffix :)