Skip to content

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

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.

Sign up for GitHub

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

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

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
45 changes: 45 additions & 0 deletions src/Essential.Diagnostics.RollingFileTraceListener/Diagnostics/RollingFileTraceListener.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.

};
TraceFormatter traceFormatter = new TraceFormatter();
private RollingTextWriter rollingTextWriter;

// Indicate whether all subdirectories in full file path
// should be checked for existence and re-created if missed
// before opening the file
private bool? createSubdirectories;

/// <summary>
/// Constructor. Writes to a rolling text file using the default name.
/// </summary>
Expand Down Expand Up @@ -111,6 +117,42 @@ public bool ConvertWriteToEvent
}
}

/// <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)

/// </summary>
[SuppressMessage("Microsoft.Usage", "CA1806:DoNotIgnoreMethodResults", MessageId = "System.Boolean.TryParse(System.String,System.Boolean@)", Justification = "Default value is acceptable if conversion fails.")]
public bool CreateSubdirectories
{
get
{
if (createSubdirectories.HasValue)
{
return createSubdirectories.Value;
}

if (Attributes.ContainsKey("createSubdirectories"))
{
bool parsed;
if (bool.TryParse(Attributes["createSubdirectories"], out parsed))
{
createSubdirectories = parsed;
return createSubdirectories.Value;
}
}

// Default behaviour is do NOT check and fail if any of
// the subdirectories in log file path are not exists.
return false;
}
set
{
createSubdirectories = value;
Attributes["createSubdirectories"] = value.ToString(CultureInfo.InvariantCulture);
}
}

/// <summary>
/// Gets or sets the file system to use; this defaults to an adapter for System.IO.File.
/// </summary>
Expand Down Expand Up @@ -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 •
edited
Loading

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;
  }
}

rollingTextWriter.Write(null, message);
}
}
Expand All @@ -235,6 +278,7 @@ protected override void WriteLine(string category, string message, object data)
}
else
{
rollingTextWriter.FileSystem.CreateSubdirectories = CreateSubdirectories;
rollingTextWriter.WriteLine(null, message);
}
}
Expand Down Expand Up @@ -266,6 +310,7 @@ protected override void WriteTrace(TraceEventCache eventCache, string source, Tr
relatedActivityId,
data
);
rollingTextWriter.FileSystem.CreateSubdirectories = CreateSubdirectories;
rollingTextWriter.WriteLine(eventCache, output);
}

Expand Down
14 changes: 14 additions & 0 deletions src/Essential.Diagnostics.RollingFileTraceListener/IO/FileSystem.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,13 @@ namespace Essential.IO
/// </summary>
public class FileSystem : IFileSystem
{
/// <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>.
/// </summary>
public bool CreateSubdirectories { get; set; }
Copy link
Owner

@sgryphon sgryphon Jul 18, 2019 •
edited
Loading

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.


/// <summary>
/// Opens a System.IO.FileStream on the specified path,
/// having the specified mode with read, write, or read/write access
Expand All @@ -19,6 +26,13 @@ public class FileSystem : IFileSystem
/// <returns></returns>
public Stream Open(string path, FileMode mode, FileAccess access, FileShare share)
{
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).

Directory.CreateDirectory(directory);
}

return File.Open(path, mode, access, share);
}
}
Expand Down
7 changes: 7 additions & 0 deletions src/Essential.Diagnostics.RollingFileTraceListener/IO/IFileSystem.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,13 @@ namespace Essential.IO
/// </summary>
public interface IFileSystem
{
/// <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>.
/// </summary>
bool CreateSubdirectories { get; set; }

/// <summary>
/// Opens a System.IO.FileStream on the specified path,
/// having the specified mode with read, write, or read/write access
Expand Down
2 changes: 2 additions & 0 deletions src/Tests/Essential.Diagnostics.RollingFileTraceListener.Tests/Utility/MockFileSystem.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.


public IList<Tuple<string,MemoryStream>> OpenedItems = new List<Tuple<string,MemoryStream>>();

public Stream Open(string path, FileMode mode, FileAccess access, FileShare share)
Expand Down