Current coverage is 81% (diff: 79%)

Merging #1878 into master will decrease coverage by <1%

@@             master      #1878   diff @@
==========================================
  Files           276        276          
  Lines         18636      18691    +55   
  Methods        2861       2862     +1   
  Messages          0          0          
  Branches       2139       2157    +18   
==========================================
+ Hits          15160      15204    +44   
- Misses         2971       2973     +2   
- Partials        505        514     +9   

Powered by Codecov. Last update c532547...4eb15e7

I think this one also fixes #298 ?

I think this one also fixes #298 ?

The MultiFileWatcher.NotifyFilters for the Reload-Timer is different from the MultiFileWatcher.NotifyFilters for the Archive-Folder. So I don't know.

Reviewed 1 of 4 files at r1, 2 of 4 files at r2.
Review status: 3 of 5 files reviewed at latest revision, 11 unresolved discussions.

src/NLog/Internal/MultiFileWatcher.cs, line 179 at r2 (raw file):

            var exception = e.GetException();
            if (exception != null)
                InternalLogger.Info(exception, "Error Watching Path {0}", watcherPath);

I think .Warn is better? (twice)

src/NLog/Internal/FileAppenders/FileAppenderCache.cs, line 122 at r2 (raw file):

                    {
                        string currentFolderPath = Path.GetDirectoryName(e.FullPath);
                        if (StringComparer.OrdinalIgnoreCase.Compare(archiveFolderPath, currentFolderPath) != 0)

why not .Equals(..., .OrdinalIgnoreCase)?

src/NLog/Internal/FileAppenders/FileAppenderCache.cs, line 124 at r2 (raw file):

                        if (StringComparer.OrdinalIgnoreCase.Compare(archiveFolderPath, currentFolderPath) != 0)
                        {
                            if ((e.ChangeType & (WatcherChangeTypes.Deleted | WatcherChangeTypes.Renamed)) != 0)

Duplicate code (4 lines)

src/NLog/Internal/FileAppenders/FileAppenderCache.cs, line 138 at r2 (raw file):

            if ((e.ChangeType & WatcherChangeTypes.Created) == WatcherChangeTypes.Created)
            {
                logFileWasArchived = true;  // Something was created in the archive folder

partly duplicate code.

src/NLog/Internal/FileAppenders/FileAppenderCache.cs, line 160 at r2 (raw file):

                            externalFileArchivingWatcher.StopWatching(directoryPath);
                    }

no need to fall back to externalFileArchivingWatcher.StopWatching(); ?

src/NLog/Internal/FileAppenders/FileAppenderCache.cs, line 176 at r2 (raw file):

            {
                logFileWasArchived = false;
                CloseAppenders("Cleanup Archive");

why the order change? Is this to prevent to preform the cleanup twice? Don't we need a lock in that case?

src/NLog/Internal/FileAppenders/FileAppenderCache.cs, line 269 at r2 (raw file):

                    var oldAppender = appenders[freeSpot];
                    appenders[freeSpot] = null;
                    CloseAppender(oldAppender, "Stale");

is this change necessary? Or an enhancement?

src/NLog/Internal/FileAppenders/FileAppenderCache.cs, line 293 at r2 (raw file):

                        externalFileArchivingWatcher.Watch(archiveFilePatternToWatch);
                    }
                    externalFileArchivingWatcher.Watch(appenderToWrite.FileName);

two watches is by design?

src/NLog/Internal/FileAppenders/FileAppenderCache.cs, line 319 at r2 (raw file):

                    var oldAppender = appenders[i];
                    appenders[i] = null;
                    CloseAppender(oldAppender, reason);

idem, is this change necessary? Or an enhancement?

src/NLog/Internal/FileAppenders/FileAppenderCache.cs, line 339 at r2 (raw file):

#endif
            {
                if (expireTime != DateTime.MinValue)

nice improvement :)

tests/NLog.UnitTests/Targets/FileTargetTests.cs, line 3300 at r2 (raw file):

                logger.Info("log to force archiving");

                LogManager.Configuration = null;    // Flush

why are we doing this if we like to flush?

Comments from Reviewable

Reviewed 2 of 4 files at r2.
Review status: all files reviewed at latest revision, 11 unresolved discussions.

Comments from Reviewable

Thanks, reviewed it!

I'm a bit cautious with the changes in the fileAppenderCache, as I don't have worked a lot with it

I'm a bit cautious with the changes in the fileAppenderCache, as I don't have worked a lot with it

I have just noticed the bug-reports about using archiving together with keepFileOpen, and gets into the locked-file-handle problems. Not high priority for me, so can easily wait for NLog 5 (or 6)

Not high priority for me, so can easily wait for NLog 5 (or 6)

Think that's not necessary, I'm just asking more questions in the review. ;)

Reviewed 1 of 2 files at r3.
Review status: all files reviewed at latest revision, 11 unresolved discussions, some commit checks failed.

Comments from Reviewable

LGTM

Review status: all files reviewed at latest revision, 11 unresolved discussions, some commit checks failed.

Comments from Reviewable

\0/ 3 closed issues \0/ \0/