-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Injection of zip-compressor for fileTarget #1423
Conversation
Update from org
…ssor to other platforms
Sounds good! Will check this in depth this week |
Please add the file headers to the new files so appveyor won't complain |
/// <summary> | ||
/// Uses .Net4.5 ZipArchive | ||
/// </summary> | ||
public class DefaultFileCompressor : IFileCompressor |
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.
- good approach
- I think is better to only include this class in .Net 45. The bool return is then not needed.
- Please use the
<see
tag forZipArchive
- change it to
internal
? (easier to maintain)
reviewed already :) It's in a good direction, and the following changes makes in perfect IMO:
|
/// | ||
/// </summary> | ||
/// <returns></returns> | ||
public static IFileCompressor GetFileCompressor() |
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.
change to property instead of 2 methods
…ressor to ZipArchiveFileCompressor to better fit in FileTarget which now has a static property DefaultCompressor as well as instance property Compressor. ZipArchiveFileCompressor now only is included by the netfx45 project.
Great to see you already had a chance to check my proposed code. Initially my intention was that IFileCompressor could be used FileTarget independently. After moving all stuff to FileTarget according to your comments, it now seems to be a pretty straightforward to use interface. To be honest i til now only hacked this stuff together w/o any actual test and now have my doubts that i actually can set that FileTarget.DefaultCompressor static property early enough, before NLog actually creates the FileTarget instances. So i guess DefaultCompressor setter would need looping over all FileTarget instances. |
It's only important to set it before writing any logevent? |
Ok, i guess i just have to set DefaultCompressor before doing any actual logging...i'm a bit unsure how the NLog bootstrap is done. |
Sure, see line 164, 171 etc: https://ci.appveyor.com/project/nlog/nlog/build/4.0.2397. Let me know if it's unclear (and ignore the typo ;)) |
…ng ZipArchiveFileCompressor.cs in all project, thus again added NET4_5 checks.
/// Per instance file compressor to be used to compress log files during archiving. | ||
/// Defaults to <see cref="DefaultCompressor"/>. | ||
/// </summary> | ||
public IFileCompressor Compressor { get; set; } |
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.
proposal: rename to FileCompressor
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 would have been my personal preference, but you proposed "Compressor" as the property name above ;)
\0/ it builds. Some last things about naming and I was a bit confused with two properties, see notes. |
I'll remove the per FileTarget Instance Compressor property. Just leaving the static property which I'll rename to FileCompressor. And I'll rename IFileCompressor.Compress() to IFileCompressor.CompressFile(). |
… the static property which was renamed to FileCompressor. Renamed IFileCompressor.Compress() to IFileCompressor.CompressFile().
Cool! Think we needs some unit tests and then we are ready to go |
I'll see if I can invent some unit tests soon. |
Good idea about supporting .7z etc. But I think already fits in the current interface? AFAIK the .zip is only used as fallback, so a user could inject how FileCompressor and configure something like "archive.7z" |
Right, in FileTarget its just a fallback. Its actually just the unit tests hardcoding .zip or .txt as archive extension. But that can stay as is...so you're right no need to have such ArchiveExtension property. |
Hi all, |
We needs some unit tests, and then it's ready for merge. After merge I expect the release within a few days. I you could help us with unit tests, then I can merge (and release) it earlier. |
OK great, How do I help you ? :) |
|
I'd do such UnitTests...just a bit bussy...my main problem just doing a simple UnitTest, ist that I wasn't easily able figuring out whether xUnit is running tests in parallel. If it would I can't just set FileTarget.FileCompressor to another IFileCompressor implementation based on e.g. DotNetZip, because that may crash the other tests also doing file with zip archiving. |
I can answer that :) We are not running tests in parallel, so you can set the static and revert with a
Thanks! |
Current coverage is 75.41%
@@ master #1423 diff @@
==========================================
Files 267 269 +2
Lines 15990 16066 +76
Methods 0 0
Messages 0 0
Branches 1724 1738 +14
==========================================
- Hits 12762 12116 -646
- Misses 3228 3528 +300
- Partials 0 422 +422
|
Codecov is lost again... |
@AndreGleichner is dotnetzip a good library? Then maybe we can release it as separate package. We can then remove it from here and replace it with a mock. |
IMO DotNetZip is one of the best pure managed zip libs. The only comparable lib is SharpZipLib. But the latter is GPL thus at work we've used DotNetZip since couple years w/o issues and its licence fits our needs. |
DotNetZip is only refed by the unit tests...didn't know adding new dependencies there could harm. |
The external lib is no problem in the unit tests :) But I think it's nice to release the implementation as separate package and then we have twice the same code and tests ;) Codecov is buggy. The -4% is wrong. More unit tests are nice, but right now it's unclear to me what isn't covered. |
Anything else i could do to get this pr merged? |
Forced a rebuild. I added an unstable test a few days ago. Not sure what's wrong. |
merged :) |
Yeah :) |
PS what is the difference between DotNetZip and DotNetZip.Reduced? |
Reduced just means it is w/o self extracting archive support.
|
This has been released with NLog 4.3.4 :) |
See also Issue #1420.
I need to get archive files zipped on .Net4.0 and thus have added a IFileCompressor interface to LogManager, so that NLog users may call NLog.LogManager.SetFileCompressor() to inject there own preferred file compressor like DotNetZip.