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 {Hour} and {HalfHour} specifiers. #29

Merged
merged 4 commits into from Oct 9, 2016
Merged
Changes from 2 commits
Commits
File filter...
Filter file types
Jump to…
Jump to file or symbol
Failed to load files and symbols.
+126 −26
Diff settings

Always

Just for now

@@ -126,11 +126,12 @@ void AlignCurrentFileTo(DateTime now)

void OpenFile(DateTime now)
{
var date = now.Date;
var currentCheckpoint = _roller.GetCurrentCheckpoint(now);

// We only take one attempt at it because repeated failures
// to open log files REALLY slow an app down.
_nextCheckpoint = date.AddDays(1);

This comment has been minimized.

Copy link
@nblumhardt

nblumhardt Oct 3, 2016

Member

Extra newline

This comment has been minimized.

Copy link
@nblumhardt

nblumhardt Oct 6, 2016

Member

Still an extra newline hanging around here.

_nextCheckpoint = _roller.GetNextCheckpoint(now);

var existingFiles = Enumerable.Empty<string>();
try
@@ -140,13 +141,13 @@ void OpenFile(DateTime now)
}
catch (DirectoryNotFoundException) { }

var latestForThisDate = _roller
var latestForThisDateTime = _roller

This comment has been minimized.

Copy link
@nblumhardt

nblumhardt Oct 3, 2016

Member

Would it be clearer if we called this latestForThisCheckpoint?

This comment has been minimized.

Copy link
@jose3m

jose3m Oct 4, 2016

Author

Feel free to rename any field or constant.

.SelectMatches(existingFiles)
.Where(m => m.Date == date)
.Where(m => m.DateTime == currentCheckpoint)
.OrderByDescending(m => m.SequenceNumber)
.FirstOrDefault();

var sequence = latestForThisDate != null ? latestForThisDate.SequenceNumber : 0;
var sequence = latestForThisDateTime != null ? latestForThisDateTime.SequenceNumber : 0;

const int maxAttempts = 3;
for (var attempt = 0; attempt < maxAttempts; attempt++)
@@ -196,7 +197,7 @@ void ApplyRetentionPolicy(string currentFilePath)

var newestFirst = _roller
.SelectMatches(potentialMatches)
.OrderByDescending(m => m.Date)
.OrderByDescending(m => m.DateTime)
.ThenByDescending(m => m.SequenceNumber)
.Select(m => m.Filename);

@@ -18,16 +18,16 @@ namespace Serilog.Sinks.RollingFile
{
class RollingLogFile
{
public RollingLogFile(string filename, DateTime date, int sequenceNumber)
public RollingLogFile(string filename, DateTime dateTime, int sequenceNumber)
{
Filename = filename;
Date = date;
DateTime = dateTime;
SequenceNumber = sequenceNumber;
}

public string Filename { get; }

public DateTime Date { get; }
public DateTime DateTime { get; }

public int SequenceNumber { get; }
}
@@ -29,18 +29,42 @@ class TemplatedPathRoller
const string OldStyleDateSpecifier = "{0}";
const string DateSpecifier = "{Date}";
const string DateFormat = "yyyyMMdd";
const string HourSpecifier = "{Hour}";
const string HourFormat = "yyyyMMddHH";
const string HalfHourSpecifier = "{HalfHour}";
const string HalfHourFormat = "yyyyMMddHHmm";
const string DefaultSeparator = "-";

const string MatcherMarkSpecifier = "date";

This comment has been minimized.

Copy link
@nblumhardt

nblumhardt Oct 3, 2016

Member

Good idea pulling these out as constants 👍

Just on the naming, could we perhaps use:

const string SpecifierMatchGroup = "specifier";
const string SequenceNumberMatchGroup" = "sequence";
const string MatcherMarkInc = "inc";

readonly string _pathTemplate;
readonly Regex _filenameMatcher;
readonly SpecifierTypeEnum _specifierType = SpecifierTypeEnum.None;

This comment has been minimized.

Copy link
@nblumhardt

nblumhardt Oct 3, 2016

Member

Carrying both _specifierType and _usedSpecifier means the two could (potentially) be out-of-sync. What do you think about getting rid of the enum, and just using e.g. _usedSpecifier == HourSpecifier etc. as comparisons?

This comment has been minimized.

Copy link
@nblumhardt

nblumhardt Oct 3, 2016

Member

One other option that would work really well:

class Specifier
{
    public string Name { get; }
    public string Token { get; }
    public string Format { get; }
    public TimeSpan Interval { get; }

    Specifier(string name, string format, string interval)
    {
        Name = name;
        Token = "{" + Name + "}";
        Format = format;
        Interval = interval;
    }

    public static readonly Specifier Date = new Specifier("Date", "yyyyMMdd", TimeSpan.FromDays(1));
    public static readonly Specifier Hour = new Specifier("Hour", "yyyyMMddhh", TimeSpan.FromHours(1));
    public static readonly Specifier HalfHour = new Specifier("HalfHour", "yyyyMMddhhmm", TimeSpan.FromMinutes(30));
}

We could then have a field:

readonly Specifier _specifier;

Then, after construction, we could avoid checking the current specifier and just use fields like _specifier.Token, _specifier.Interval and so-on.

What do you think?

This comment has been minimized.

Copy link
@jose3m

jose3m Oct 4, 2016

Author

For me it is great to encapsulate the fields in a class.
I didn't want to make so much changes from the original source so the merge should be easier.
Feel free to make any change with which you feel more confortable.

// Concret used Date or Hour specifier.

This comment has been minimized.

Copy link
@nblumhardt

nblumhardt Oct 3, 2016

Member

Typo in this comment ("concret") - we could possibly drop the comment since the field names are pretty descriptive.

This comment has been minimized.

Copy link
@jose3m

jose3m Oct 4, 2016

Author

Perfect.

readonly string _usedSpecifier = string.Empty;
readonly string _usedFormat = string.Empty;

public TemplatedPathRoller(string pathTemplate)
{
if (pathTemplate == null) throw new ArgumentNullException(nameof(pathTemplate));

if (pathTemplate.Contains(OldStyleDateSpecifier))
throw new ArgumentException("The old-style date specifier " + OldStyleDateSpecifier +
" is no longer supported, instead please use " + DateSpecifier);

int numSpecifiers = 0;
if (pathTemplate.Contains(DateSpecifier))
numSpecifiers++;
if (pathTemplate.Contains(HourSpecifier))
numSpecifiers++;
if (pathTemplate.Contains(HalfHourSpecifier))
numSpecifiers++;
if (numSpecifiers > 1)
throw new ArgumentException("The date, hour and half-hour specifiers (" +
DateSpecifier + "," + HourSpecifier + "," + HalfHourSpecifier +
") cannot be used at the same time");

var directory = Path.GetDirectoryName(pathTemplate);
if (string.IsNullOrEmpty(directory))
{
@@ -51,26 +75,57 @@ public TemplatedPathRoller(string pathTemplate)

if (directory.Contains(DateSpecifier))
throw new ArgumentException("The date cannot form part of the directory name");
if (directory.Contains(HourSpecifier))
throw new ArgumentException("The hour specifiers cannot form part of the directory name");
if (directory.Contains(HalfHourSpecifier))
throw new ArgumentException("The half-hour specifiers cannot form part of the directory name");

var filenameTemplate = Path.GetFileName(pathTemplate);
if (!filenameTemplate.Contains(DateSpecifier))
if (!filenameTemplate.Contains(DateSpecifier) &&
!filenameTemplate.Contains(HourSpecifier) &&
!filenameTemplate.Contains(HalfHourSpecifier))
{
// If the file name doesn't use any of the admitted specifiers then it is added the date specifier
// as de default one.
filenameTemplate = Path.GetFileNameWithoutExtension(filenameTemplate) + DefaultSeparator +
DateSpecifier + Path.GetExtension(filenameTemplate);
}

var indexOfSpecifier = filenameTemplate.IndexOf(DateSpecifier, StringComparison.Ordinal);
//---
// From this point forward we don't reference the Date or Hour concret specifiers and formats :
// we will reference only the one set as "used" (_usedSpecifier and _usedFormat).

if (filenameTemplate.Contains(DateSpecifier))
{
_usedSpecifier = DateSpecifier;
_usedFormat = DateFormat;
_specifierType = SpecifierTypeEnum.Date;
}
else if (filenameTemplate.Contains(HourSpecifier))
{
_usedSpecifier = HourSpecifier;
_usedFormat = HourFormat;
_specifierType = SpecifierTypeEnum.Hour;
}
else if (filenameTemplate.Contains(HalfHourSpecifier))
{
_usedSpecifier = HalfHourSpecifier;
_usedFormat = HalfHourFormat;
_specifierType = SpecifierTypeEnum.HalfHour;
}

var indexOfSpecifier = filenameTemplate.IndexOf(_usedSpecifier, StringComparison.Ordinal);
var prefix = filenameTemplate.Substring(0, indexOfSpecifier);
var suffix = filenameTemplate.Substring(indexOfSpecifier + DateSpecifier.Length);
var suffix = filenameTemplate.Substring(indexOfSpecifier + _usedSpecifier.Length);
_filenameMatcher = new Regex(
"^" +
Regex.Escape(prefix) +
"(?<date>\\d{" + DateFormat.Length + "})" +
"(?<inc>_[0-9]{3,}){0,1}" +
"(?<" + MatcherMarkSpecifier + ">\\d{" + _usedFormat.Length + "})" +
"(?<" + MatcherMarkInc + ">_[0-9]{3,}){0,1}" +
Regex.Escape(suffix) +
"$");

DirectorySearchPattern = filenameTemplate.Replace(DateSpecifier, "*");
DirectorySearchPattern = filenameTemplate.Replace(_usedSpecifier, "*");
LogFileDirectory = directory;
_pathTemplate = Path.Combine(LogFileDirectory, filenameTemplate);
}
@@ -81,12 +136,14 @@ public TemplatedPathRoller(string pathTemplate)

public void GetLogFilePath(DateTime date, int sequenceNumber, out string path)
{
var tok = date.ToString(DateFormat, CultureInfo.InvariantCulture);
DateTime currentCheckpoint = GetCurrentCheckpoint(date);

This comment has been minimized.

Copy link
@nblumhardt

nblumhardt Oct 6, 2016

Member

Nitpick, can use var here.

This comment has been minimized.

Copy link
@jose3m

var tok = currentCheckpoint.ToString(_usedFormat, CultureInfo.InvariantCulture);

if (sequenceNumber != 0)
tok += "_" + sequenceNumber.ToString("000", CultureInfo.InvariantCulture);

path = _pathTemplate.Replace(DateSpecifier, tok);
path = _pathTemplate.Replace(_usedSpecifier, tok);
}

public IEnumerable<RollingLogFile> SelectMatches(IEnumerable<string> filenames)
@@ -97,26 +154,68 @@ public IEnumerable<RollingLogFile> SelectMatches(IEnumerable<string> filenames)
if (match.Success)
{
var inc = 0;
var incGroup = match.Groups["inc"];
var incGroup = match.Groups[MatcherMarkInc];
if (incGroup.Captures.Count != 0)
{
var incPart = incGroup.Captures[0].Value.Substring(1);
inc = int.Parse(incPart, CultureInfo.InvariantCulture);
}

DateTime date;
var datePart = match.Groups["date"].Captures[0].Value;
DateTime dateTime;
var dateTimePart = match.Groups[MatcherMarkSpecifier].Captures[0].Value;
if (!DateTime.TryParseExact(
datePart,
DateFormat,
dateTimePart,
_usedFormat,
CultureInfo.InvariantCulture,
DateTimeStyles.None,
out date))
out dateTime))
continue;

yield return new RollingLogFile(filename, date, inc);
yield return new RollingLogFile(filename, dateTime, inc);
}
}
}

public DateTime GetCurrentCheckpoint(DateTime date)

This comment has been minimized.

Copy link
@nblumhardt

nblumhardt Oct 3, 2016

Member

Could we call the parameter name instant or dateTime?

This comment has been minimized.

Copy link
@jose3m

jose3m Oct 4, 2016

Author

"instant" is nice for me. More descriptive than "datetime" or just "date".

{
if (_specifierType == SpecifierTypeEnum.Hour)
{
return date.Date.AddHours(date.Hour);
}
else if (_specifierType == SpecifierTypeEnum.HalfHour)
{
DateTime auxDT = date.Date.AddHours(date.Hour);
if (date.Minute >= 30)
auxDT = auxDT.AddMinutes(30);
return auxDT;
}

return date.Date;
}

public DateTime GetNextCheckpoint(DateTime date)

This comment has been minimized.

Copy link
@nblumhardt

nblumhardt Oct 3, 2016

Member

Parameter naming here also.

{
DateTime currentCheckpoint = GetCurrentCheckpoint(date);

if (_specifierType == SpecifierTypeEnum.Hour)
{
return currentCheckpoint.AddHours(1);
}
else if (_specifierType == SpecifierTypeEnum.HalfHour)
{
return currentCheckpoint.AddMinutes(30);
}

return currentCheckpoint.AddDays(1);
}
}
}

enum SpecifierTypeEnum

This comment has been minimized.

Copy link
@nblumhardt

nblumhardt Oct 3, 2016

Member

If we decide to keep the enum (see comment above), we should drop the Enum suffix. Just calling it Specifier would be clear enough.

This comment has been minimized.

Copy link
@jose3m

jose3m Oct 4, 2016

Author

I would prefer to use a class for the specifier. It is more OOP compliant.

{
None,
Date,
Hour,
HalfHour
}

}
@@ -121,7 +121,7 @@ public void MatchingParsesDates()
var roller = new TemplatedPathRoller("log-{Date}.txt");
const string newer = "log-20150101.txt";
const string older = "log-20141231.txt";
var matched = roller.SelectMatches(new[] { older, newer }).OrderByDescending(m => m.Date).Select(m => m.Filename).ToArray();
var matched = roller.SelectMatches(new[] { older, newer }).OrderByDescending(m => m.DateTime).Select(m => m.Filename).ToArray();
Assert.Equal(new[] { newer, older }, matched);
}
}
ProTip! Use n and p to navigate between commits in a pull request.
You can’t perform that action at this time.