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

Conversation

Projects
None yet
2 participants
@jose3m
Copy link

jose3m commented Sep 29, 2016

This PR corresponds to the issue #22 .

This development adds two new specifiers to the currently admitted {Date}: {Hour} and {HalfHour}
The behaviour is obvius.

Please, feel free to change the token {HalfHour} by the one you consider better (you only have to change the value of the constant TemplatedPathRoller.HalfHourSpecifier).

One thing to have in mind when using it is that the three specifiers shouldn't be used at the same or different time on the same path as it could be confusing with an unexpected result.

Thanks.
Jose

jose3m added some commits Sep 29, 2016

@nblumhardt

This comment has been minimized.

Copy link
Member

nblumhardt commented Sep 30, 2016

Great! Thanks for the PR. It may take me a few days to work through it, just letting you know that it hasn't fallen off the radar :-)

@jose3m

This comment has been minimized.

Copy link
Author

jose3m commented Sep 30, 2016

I'll be expecting it !

Thanks a lot.

@nblumhardt
Copy link
Member

nblumhardt left a comment

Looks great. Added a few comments. Thanks!


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

@nblumhardt

nblumhardt Oct 3, 2016

Member

Extra newline

This comment has been minimized.

@nblumhardt

nblumhardt Oct 6, 2016

Member

Still an extra newline hanging around here.

@@ -140,13 +141,13 @@ void OpenFile(DateTime now)
}
catch (DirectoryNotFoundException) { }

var latestForThisDate = _roller
var latestForThisDateTime = _roller

This comment has been minimized.

@nblumhardt

nblumhardt Oct 3, 2016

Member

Would it be clearer if we called this latestForThisCheckpoint?

This comment has been minimized.

@jose3m

jose3m Oct 4, 2016

Author

Feel free to rename any field or constant.

const string DefaultSeparator = "-";

const string MatcherMarkSpecifier = "date";

This comment has been minimized.

@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";
readonly string _pathTemplate;
readonly Regex _filenameMatcher;
readonly SpecifierTypeEnum _specifierType = SpecifierTypeEnum.None;
// Concret used Date or Hour specifier.

This comment has been minimized.

@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.

@jose3m

jose3m Oct 4, 2016

Author

Perfect.

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

This comment has been minimized.

@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.

@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.

@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.


enum SpecifierTypeEnum

This comment has been minimized.

@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.

@jose3m

jose3m Oct 4, 2016

Author

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

}
}
}

public DateTime GetCurrentCheckpoint(DateTime date)

This comment has been minimized.

@nblumhardt

nblumhardt Oct 3, 2016

Member

Could we call the parameter name instant or dateTime?

This comment has been minimized.

@jose3m

jose3m Oct 4, 2016

Author

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

return date.Date;
}

public DateTime GetNextCheckpoint(DateTime date)

This comment has been minimized.

@nblumhardt

nblumhardt Oct 3, 2016

Member

Parameter naming here also.

@nblumhardt

This comment has been minimized.

Copy link
Member

nblumhardt commented Oct 4, 2016

Thanks for the replies @jose3m!

In the PR workflow, you can push additional changes to your repository and they'll appear here in the PR. (As the code's still on your side, I can't make changes to it "on the way in".)

Cheers,
Nick

@jose3m

This comment has been minimized.

Copy link
Author

jose3m commented Oct 5, 2016

Hi Nick !

I have just commited the changes we talked about.
I have added more code to the Specifier class as it will be better encapsulated.

Thanks.
Jose

@nblumhardt
Copy link
Member

nblumhardt left a comment

Looks like we're getting close. Mostly stylistic feedback. There seem to be a few opportunities to encapsulate more in the Specifier class - what do you think? Cheers!


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

@nblumhardt

nblumhardt Oct 6, 2016

Member

Still an extra newline hanging around here.


namespace Serilog.Sinks.RollingFile.Sinks.RollingFile
{
internal class Specifier

This comment has been minimized.

@nblumhardt

nblumhardt Oct 6, 2016

Member

Nitpick - the codebase doesn't use explicit internal modifiers on types or private modifiers on members.

This comment has been minimized.

@jose3m
internal static readonly TimeSpan HourInterval = TimeSpan.FromHours(1);
internal static readonly TimeSpan HalfHourInterval = TimeSpan.FromMinutes(30);

//-------------------

This comment has been minimized.

@nblumhardt

nblumhardt Oct 6, 2016

Member

//---- and whitespace can probably go, not used through the rest of the Serilog codebase.

This comment has been minimized.

@jose3m
public string Format { get; }
public TimeSpan Interval { get; }

Specifier(SpecifierType type)

This comment has been minimized.

@nblumhardt

nblumhardt Oct 6, 2016

Member

It seems like SpecifierType only exists to use in this constructor. If the ctor took name, format and interval, we could get rid of SpecifierType and save quite a few LOC (including the switch statement).

This comment has been minimized.

@jose3m

jose3m Oct 6, 2016

Author

SpecifierType is used also in the getCurrentCheckpoint function. I'll remove this enum and I'll use the token to identify what kind of specifier is the instance.

{
internal const string OldStyleDateToken = "{0}";

internal const string DateToken = "{Date}";

This comment has been minimized.

@nblumhardt

nblumhardt Oct 6, 2016

Member

It will be tidier to expose the specifiers instead, so these can just be Specifier.Date.Token.

This comment has been minimized.

@jose3m

jose3m Oct 6, 2016

Author

I agree. I change it.

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

if (pathTemplate.Contains(Specifier.OldStyleDateToken))

This comment has been minimized.

@nblumhardt

nblumhardt Oct 6, 2016

Member

The next 13 lines could be encapsulated in Specifier.TryGetSpecifier(pathTemplate, out specifier). If it returns false, _specifier can be set to Specifier.Date and the path modified. TryGetSpecifier() could then also be used to avoid the chain of three if statements to decide whether it's included in the directory name.

@@ -49,28 +65,42 @@ public TemplatedPathRoller(string pathTemplate)

directory = Path.GetFullPath(directory);

if (directory.Contains(DateSpecifier))
if (directory.Contains(Specifier.DateToken))

This comment has been minimized.

@nblumhardt

nblumhardt Oct 6, 2016

Member

E.g.

Specifier directorySpecifier;
if (Specifier.TryGetSpecifier(directory, out directorySpecifier)
{
    throw new ArgumentException($"The {directorySpecifer.Token} specifier cannot form part of the directory name.");
}

This comment has been minimized.

@jose3m
@@ -81,12 +111,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.

@nblumhardt

nblumhardt Oct 6, 2016

Member

Nitpick, can use var here.

This comment has been minimized.

@jose3m
@jose3m

This comment has been minimized.

Copy link
Author

jose3m commented Oct 6, 2016

Hi Nick !
Last changes are commited.

Even more encapsulating, it could be created a new static method inside Specifier containing all validations on the TemplatedPathRoller constructor, from line 42 to line 72.

Specifier.ValidateTemplate(string pathTemplate, out string directory)

Regards
Jose

@nblumhardt

This comment has been minimized.

Copy link
Member

nblumhardt commented Oct 9, 2016

Thanks Jose! I will merge it now and have a tinker with the encapsulated validation myself. Cheers!

@nblumhardt nblumhardt merged commit a7a7a1d into serilog:dev Oct 9, 2016

1 check passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
@jose3m

This comment has been minimized.

Copy link
Author

jose3m commented Oct 10, 2016

Thank you very much for your great work !

@nblumhardt nblumhardt referenced this pull request Oct 24, 2016

Merged

3.2.0 Release #35

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.