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

- added "CreateSubdirectories" parameter to the "RollingFileTraceListner" #32

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

sevenate
Copy link

Added CreateSubdirectories parameter to the RollingFileTraceListener with default set to false to mimic current behavior of the listener (which will throw DirectoryNotFoundException in case of any sub-directories of the full file path are not exist in the system), but setting it to true make it possible to have initializeData template specified for example like this:

{AppData}\LogFiles\{DateTime:yyyy-MM}\{DateTime:dd}\YourApplication.log

…ener" with default set to "false" to mimic current behaviour of the listener (and throw DirectoryNotFoundException in case of any sub-directories of the full file path are not exist in the system), but setting it to "true" make it possible to have initializeData template specified like "{AppData}\LogFiles\{DateTime:yyyy-MM}\{DateTime:dd}\YourApplication.log"
/// <summary>
/// Gets or sets the value indicating whether all subdirectories in full file path
/// should be checked for existence and re-created if missed
/// before opening the file. Default value is <c>False</c>.
Copy link
Owner

Choose a reason for hiding this comment

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

Make the default value True; it is adding a capability and doesn't break anything existing. (The existing error throwing is not a feature)

@@ -218,6 +260,7 @@ protected override void Write(string category, string message, object data)
}
else
{
rollingTextWriter.FileSystem.CreateSubdirectories = CreateSubdirectories;
Copy link
Owner

@sgryphon sgryphon Jul 18, 2019

Choose a reason for hiding this comment

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

Rather than set the value on every write/flush, it would be better to have lazy creation (instead of in the constructor) of the rolling text writer (with double checking locking for multi-threaded), and pass Create into the constructor.

There are actually 2 PRs for adding features to the rolling writer, so setting it up with lazy creation will make it easier to add multiple configurable capabilities.

e.g.

protected RollingTextWriter RollingTextWriter { 
  get {
    if (rollingTextWriter == null)
    {
       lock (rollingTextWriterLock)
       {
          if (rollingTextWriter == null)
          {
             rollingTextWriter = new RollingTextWriter (... path ..., CreateDirectories);
          }
        }
      }
    }
    return rollingTextWriter;
  }
}

/// should be checked for existence and re-created if missed
/// before opening the file. Default value is <c>False</c>.
/// </summary>
public bool CreateSubdirectories { get; set; }
Copy link
Owner

@sgryphon sgryphon Jul 18, 2019

Choose a reason for hiding this comment

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

As noted in the description of FileSystem / IFileSystem, it is meant to be a wrapper around System.IO.File to allow substitution during testing.

It should not be extended with new properties that implement business logic, such as CreateSubdirectories. The Create property should be on the RollingTextWriter class, and it control the logic of which File methods to call.

Move property to RollingTextWriter.

if (CreateSubdirectories)
{
// Making sure that all subdirectories in file path exists
var directory = Path.GetDirectoryName(path);
Copy link
Owner

Choose a reason for hiding this comment

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

The IFileSystem interface should be a wrapper around the IO static methods, so they can be substituted for testing.

i.e. Add methods to the interface for GetDirectoryName() and CreateDirectory(), each of which forwards directly to the corresponding static method.

e.g.

        public DirectoryInfo CreateDirectory(string path)
        {
            return Directory.Create(path);
        }

Do the same for GetDirectoryName(), and add doc comments (copy them from the .NET Framework documentation).

Keep the wrapper very simple, because it can't be tested, the IFileSystem is replaced in unit tests.

Business logic, such as "if (flag) { get directory name, create directory }" would then be in the RollingTextWriter (and can be tested).

@@ -7,6 +7,8 @@ namespace Essential.Diagnostics.Tests.Utility
{
public class MockFileSystem : IFileSystem
{
public bool CreateSubdirectories { get; set; }
Copy link
Owner

Choose a reason for hiding this comment

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

As mentioned above, mock out the GetDirectoryName (can just forward to the real one) and CreateDirectory (keep a list of created) methods, and use them to test the logic.

@sgryphon
Copy link
Owner

Nice idea to extend the template to include values in the path; will be very useful for organising log files.

Comments added to some changes in implementation before I merge; mostly about keeping IFileSystem a pure wrapper for the static System.IO methods, and moving logic into the writer.

@@ -37,10 +37,16 @@ public class RollingFileTraceListener : TraceListenerBase
{
"template", "Template",
"convertWriteToEvent", "ConvertWriteToEvent",
"createSubdirectories", "CreateSubdirectories"
Copy link
Owner

Choose a reason for hiding this comment

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

Just call it createDirectories / CreateDirectories, as it will actually create all directories (whether they are subdirectories or top level directories). Most of the time it will be used to only create subdirectories, but it will actually create both, so should be named accordingly.

Copy link
Owner

@sgryphon sgryphon left a comment

Choose a reason for hiding this comment

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

Nice idea to extend the template to include values in the path; will be very useful for organising log files.

Comments added to some changes in implementation before I merge; mostly about keeping IFileSystem a pure wrapper for the static System.IO methods, and moving logic into the writer.

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.

None yet

2 participants