You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@logging.apache.org by EbenZhang <gi...@git.apache.org> on 2017/07/27 14:10:05 UTC
[GitHub] logging-log4net pull request #13: Support combination of roll by size, prese...
GitHub user EbenZhang opened a pull request:
https://github.com/apache/logging-log4net/pull/13
Support combination of roll by size, preserve file extension and folder
[LOG4NET-571](https://issues.apache.org/jira/browse/LOG4NET-571)
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/Nicologies/logging-log4net develop
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/logging-log4net/pull/13.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #13
----
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] logging-log4net issue #13: Support combination of roll by size, preserve fil...
Posted by dpsenner <gi...@git.apache.org>.
Github user dpsenner commented on the issue:
https://github.com/apache/logging-log4net/pull/13
It was fixed in develop and then merged into the feature branches. Please base your changes on develop. It is up to you if you woukd like to create a feature branch for your changes. It would be sensible to do so if you plan to work on other issues as well.
Ng stands for next generation.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] logging-log4net issue #13: Support combination of roll by size, preserve fil...
Posted by EbenZhang <gi...@git.apache.org>.
Github user EbenZhang commented on the issue:
https://github.com/apache/logging-log4net/pull/13
Done
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] logging-log4net pull request #13: Support combination of roll by size, prese...
Posted by dpsenner <gi...@git.apache.org>.
Github user dpsenner commented on a diff in the pull request:
https://github.com/apache/logging-log4net/pull/13#discussion_r130216707
--- Diff: tests/src/Appender/RollingFileAppenderTest.cs ---
@@ -103,7 +104,8 @@ private static void ResetAndDeleteTestFiles()
Utils.GetRepository().Shutdown();
((Repository.Hierarchy.Hierarchy)Utils.GetRepository()).Clear();
- DeleteTestFiles();
+ DeleteTestFiles(c_fileName);
--- End diff --
I would like to preserve the method with signature `private static void DeleteTestFiles()` and move these two invocations there.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] logging-log4net pull request #13: Support combination of roll by size, prese...
Posted by dpsenner <gi...@git.apache.org>.
Github user dpsenner commented on a diff in the pull request:
https://github.com/apache/logging-log4net/pull/13#discussion_r129869327
--- Diff: tests/src/Appender/RollingFileAppenderTest.cs ---
@@ -239,13 +251,50 @@ public void RollingCombinedWithPreserveExtension()
}
VerifyFileCount(2, true);
}
+ [Test]
+ public void RollingBySizePreserveExtensionAndWithFolder()
+ {
+ _root = ((Repository.Hierarchy.Hierarchy)Utils.GetRepository()).Root;
+ _root.Level = Level.All;
+ PatternLayout patternLayout = new PatternLayout();
+ patternLayout.ActivateOptions();
+
+ RollingFileAppender roller = new RollingFileAppender();
+ roller.StaticLogFileName = false;
+ roller.Layout = patternLayout;
+ roller.AppendToFile = true;
+ roller.RollingStyle = RollingFileAppender.RollingMode.Size;
+ roller.MaxSizeRollBackups = 2;
+ roller.CountDirection = 1;
+ roller.PreserveLogFileNameExtension = true;
+ roller.MaximumFileSize = "5KB";
+ roller.File = c_fileNameWithFolder;
+ roller.ActivateOptions();
+ _root.AddAppender(roller);
+
+ _root.Repository.Configured = true;
+ for (int i = 0; i < 1000; i++)
+ {
+ StringBuilder s = new StringBuilder();
+ for (int j = 50; j < 100; j++)
+ {
+ if (j > 50)
+ {
+ s.Append(" ");
+ }
+ s.Append(j);
+ }
+ _root.Log(Level.Debug, s.ToString(), null);
+ }
+ VerifyFileCount(c_fileNameWithFolder, roller.MaxSizeRollBackups + 1, true);
+ }
/// <summary>
/// Removes all test files that exist
/// </summary>
- private static void DeleteTestFiles()
+ private static void DeleteTestFiles(string baseFileName)
{
- ArrayList alFiles = GetExistingFiles(c_fileName);
+ ArrayList alFiles = GetExistingFiles(baseFileName);
alFiles.AddRange(GetExistingFiles(c_fileName, true));
--- End diff --
Should this maybe read as?
```
ArrayList alFiles = GetExistingFiles(c_fileName);
alFiles.AddRange(GetExistingFiles(c_fileName, true));
alFiles.AdRange(GetExistingFiles(baseFileName));
```
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] logging-log4net issue #13: Support combination of roll by size, preserve fil...
Posted by dpsenner <gi...@git.apache.org>.
Github user dpsenner commented on the issue:
https://github.com/apache/logging-log4net/pull/13
Good catch and sorry for the juggling. Fixed in develop now.
Looks like the indentations were not only mixed tabs and spaces but there existed space indentations that were both 4 spaces and 8 spaces wide. :1st_place_medal: At least this file looks good now. I'll have to go through each file and reformat it with visual studio but this'll have to wait. Let's first box through your pull request.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] logging-log4net issue #13: Support combination of roll by size, preserve fil...
Posted by EbenZhang <gi...@git.apache.org>.
Github user EbenZhang commented on the issue:
https://github.com/apache/logging-log4net/pull/13
My branch is already based on the `Develop` branch. The whitespace issue is not fixed in `Develop` branch, see the source code on GitHub
https://raw.githubusercontent.com/apache/logging-log4net/develop/src/Appender/RollingFileAppender.cs, search for `string extension = Path.GetExtension(archiveFileBaseName);`, you will see the code is not aligned properly.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
Re: [GitHub] logging-log4net issue #13: Support combination of roll
by size, preserve fil...
Posted by Dominik Psenner <dp...@gmail.com>.
The implementation of the RollingFileAppender uses the IDateTime
interface to get the current datetime. You can mock that interface.
On 2017-09-13 14:20, EbenZhang wrote:
> Github user EbenZhang commented on the issue:
>
> https://github.com/apache/logging-log4net/pull/13
>
> >What about rolling by date in combination with subdirectories? What if the file should be rolled by date and be placed in a subdirectory that contains the rolling date?
>
> Is there a simple way to mock the date in unit test?
>
>
>
> ---
[GitHub] logging-log4net issue #13: Support combination of roll by size, preserve fil...
Posted by EbenZhang <gi...@git.apache.org>.
Github user EbenZhang commented on the issue:
https://github.com/apache/logging-log4net/pull/13
>What about rolling by date in combination with subdirectories? What if the file should be rolled by date and be placed in a subdirectory that contains the rolling date?
Is there a simple way to mock the date in unit test?
---
[GitHub] logging-log4net pull request #13: Support combination of roll by size, prese...
Posted by dpsenner <gi...@git.apache.org>.
Github user dpsenner commented on a diff in the pull request:
https://github.com/apache/logging-log4net/pull/13#discussion_r130217035
--- Diff: tests/src/Appender/RollingFileAppenderTest.cs ---
@@ -240,13 +252,52 @@ public void RollingCombinedWithPreserveExtension()
VerifyFileCount(2, true);
}
+ [Test]
+ public void RollingBySizePreserveExtensionAndWithFolder()
--- End diff --
.. or `RollingBySizeAndPreserveExtensionShouldWorkWithSubFolders` because it is shorter?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] logging-log4net pull request #13: Support combination of roll by size, prese...
Posted by EbenZhang <gi...@git.apache.org>.
GitHub user EbenZhang reopened a pull request:
https://github.com/apache/logging-log4net/pull/13
Support combination of roll by size, preserve file extension and folder
[LOG4NET-571](https://issues.apache.org/jira/browse/LOG4NET-571)
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/Nicologies/logging-log4net develop
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/logging-log4net/pull/13.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #13
----
commit f939aea483c33f8e78c35426af97e11ed9be0a57
Author: EbenZhang <zh...@gmail.com>
Date: 2017-07-27T13:46:23Z
Support combination of roll by size, preserve file extension and folder
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] logging-log4net pull request #13: Support combination of roll by size, prese...
Posted by EbenZhang <gi...@git.apache.org>.
Github user EbenZhang closed the pull request at:
https://github.com/apache/logging-log4net/pull/13
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
Re: [GitHub] logging-log4net issue #13: Support combination of roll
by size, preserve fil...
Posted by Dominik Psenner <dp...@gmail.com>.
Take your time.
2017-08-06 15:31 GMT+02:00 EbenZhang <gi...@git.apache.org>:
> Github user EbenZhang commented on the issue:
>
> https://github.com/apache/logging-log4net/pull/13
>
> was extremely busy these days.days will have a look of your comments
> as soon as I get chance.
>
>
> ---
> If your project is set up for it, you can reply to this email and have your
> reply appear on GitHub as well. If your project does not have this feature
> enabled and wishes so, or if the feature is enabled but not working, please
> contact infrastructure at infrastructure@apache.org or file a JIRA ticket
> with INFRA.
> ---
>
--
Dominik Psenner
[GitHub] logging-log4net issue #13: Support combination of roll by size, preserve fil...
Posted by EbenZhang <gi...@git.apache.org>.
Github user EbenZhang commented on the issue:
https://github.com/apache/logging-log4net/pull/13
was extremely busy these days.days will have a look of your comments as soon as I get chance.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] logging-log4net pull request #13: Support combination of roll by size, prese...
Posted by dpsenner <gi...@git.apache.org>.
Github user dpsenner commented on a diff in the pull request:
https://github.com/apache/logging-log4net/pull/13#discussion_r130216751
--- Diff: tests/src/Appender/RollingFileAppenderTest.cs ---
@@ -155,7 +157,17 @@ private static void VerifyFileCount(int iExpectedCount)
/// <param name="iExpectedCount"></param>
--- End diff --
Couldn't we count both files that match `c_fileName` and `c_fileNameWithFolder`? This way we would only have one `VerifyFileCount([...])` to invoke from the tests, just like there is one `DeleteTestFiles()`. This should work fine since the tests also remove both kinds of logfiles.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] logging-log4net issue #13: Support combination of roll by size, preserve fil...
Posted by EbenZhang <gi...@git.apache.org>.
Github user EbenZhang commented on the issue:
https://github.com/apache/logging-log4net/pull/13
> I fixed all whitespace issues in the develop branch
seems you have fixed it in the RollingFileAppenderNG branch, not the develop, should I base on RollingFileAppenderNG? Btw what does NG mean?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] logging-log4net pull request #13: Support combination of roll by size, prese...
Posted by dpsenner <gi...@git.apache.org>.
Github user dpsenner commented on a diff in the pull request:
https://github.com/apache/logging-log4net/pull/13#discussion_r130216842
--- Diff: tests/src/Appender/RollingFileAppenderTest.cs ---
@@ -240,13 +252,52 @@ public void RollingCombinedWithPreserveExtension()
VerifyFileCount(2, true);
}
+ [Test]
+ public void RollingBySizePreserveExtensionAndWithFolder()
+ {
+ _root = ((Repository.Hierarchy.Hierarchy)Utils.GetRepository()).Root;
+ _root.Level = Level.All;
+ PatternLayout patternLayout = new PatternLayout();
+ patternLayout.ActivateOptions();
+
+ RollingFileAppender roller = new RollingFileAppender();
+ roller.StaticLogFileName = false;
+ roller.Layout = patternLayout;
+ roller.AppendToFile = true;
+ roller.RollingStyle = RollingFileAppender.RollingMode.Size;
+ roller.MaxSizeRollBackups = 2;
+ roller.CountDirection = 1;
+ roller.PreserveLogFileNameExtension = true;
+ roller.MaximumFileSize = "5KB";
+ roller.File = c_fileNameWithFolder;
+ roller.ActivateOptions();
+ _root.AddAppender(roller);
+
+ _root.Repository.Configured = true;
+
+ for (int i = 0; i < 1000; i++)
+ {
+ StringBuilder s = new StringBuilder();
+ for (int j = 50; j < 100; j++)
+ {
+ if (j > 50)
+ {
+ s.Append(" ");
+ }
+ s.Append(j);
+ }
+ _root.Log(Level.Debug, s.ToString(), null);
+ }
+ VerifyFileCount(c_fileNameWithFolder, roller.MaxSizeRollBackups + 1, true);
--- End diff --
This should probably read as `VerifyFileCount(roller.MaxSizeRollBackups + 1, true);`
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] logging-log4net issue #13: Support combination of roll by size, preserve fil...
Posted by EbenZhang <gi...@git.apache.org>.
Github user EbenZhang commented on the issue:
https://github.com/apache/logging-log4net/pull/13
Added couple of unit tests for the subfolder. In order to easily write new unit tests, I did some refactoring.
---
[GitHub] logging-log4net pull request #13: Support combination of roll by size, prese...
Posted by dpsenner <gi...@git.apache.org>.
Github user dpsenner commented on a diff in the pull request:
https://github.com/apache/logging-log4net/pull/13#discussion_r130215532
--- Diff: src/Appender/RollingFileAppender.cs ---
@@ -1488,7 +1488,8 @@ protected void RollOverRenameFiles(string baseFileName)
int lastDotIndex = baseName.LastIndexOf(".");
if (lastDotIndex >= 0)
{
- archiveFileBaseName = baseName.Substring(0, lastDotIndex) + extension;
+ string dir = Path.GetDirectoryName(archiveFileBaseName);
--- End diff --
dir could be archiveFileBaseDir
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] logging-log4net pull request #13: Support combination of roll by size, prese...
Posted by dpsenner <gi...@git.apache.org>.
Github user dpsenner commented on a diff in the pull request:
https://github.com/apache/logging-log4net/pull/13#discussion_r130216572
--- Diff: tests/src/Appender/RollingFileAppenderTest.cs ---
@@ -240,13 +252,52 @@ public void RollingCombinedWithPreserveExtension()
VerifyFileCount(2, true);
}
+ [Test]
+ public void RollingBySizePreserveExtensionAndWithFolder()
--- End diff --
`RollingBySizePreserveExtensionAndWithFolder` should probably read as `RollingBySizeAndPreserveExtensionShouldWorkWithSubdirectories` and have a doc string that explains further what the test is about.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---