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

Fixed width functionality for output level names #711

Merged
merged 4 commits into from Apr 19, 2016

Conversation

Projects
None yet
2 participants
@iamkoch
Member

iamkoch commented Apr 4, 2016

#574 Now supports {Level,1} style syntax. Also supports upper/lower variations.

@nblumhardt

This comment has been minimized.

Show comment
Hide comment
@nblumhardt

nblumhardt Apr 4, 2016

Member

This is awesome, thanks Anthony!

I've added a few comments on the implementation, but I think overall the API and functionality are perfect.

Avoiding allocations is the name of the game here, so most of my comments are along those lines.

If OutputProperties.GetOutputProperties() could return the custom level property value, we'll cut out the need for IsLevelProperty on the core PropertyToken type, which is a bit of a backreference in the current factoring scheme.

It seems that because there's an alignment involved, the if block will then look something like:

else if (pt.Value is LevelPropertyValue)
{
   var lv = (LevelPropertyValue)pt.Value;
   lvt.Render(output, pt.Alignment, pt.Format, _formatProvider);
}

It breaks the abstraction around pt.Render() but it's a bit clearer and cheaper. What do you think?

Member

nblumhardt commented Apr 4, 2016

This is awesome, thanks Anthony!

I've added a few comments on the implementation, but I think overall the API and functionality are perfect.

Avoiding allocations is the name of the game here, so most of my comments are along those lines.

If OutputProperties.GetOutputProperties() could return the custom level property value, we'll cut out the need for IsLevelProperty on the core PropertyToken type, which is a bit of a backreference in the current factoring scheme.

It seems that because there's an alignment involved, the if block will then look something like:

else if (pt.Value is LevelPropertyValue)
{
   var lv = (LevelPropertyValue)pt.Value;
   lvt.Render(output, pt.Alignment, pt.Format, _formatProvider);
}

It breaks the abstraction around pt.Render() but it's a bit clearer and cheaper. What do you think?

@iamkoch

This comment has been minimized.

Show comment
Hide comment
@iamkoch

iamkoch Apr 5, 2016

Member

I had what you describe originally, however the render method with zero functionality was what put me off. If you're OK with a no op I'll switch it back

Member

iamkoch commented Apr 5, 2016

I had what you describe originally, however the render method with zero functionality was what put me off. If you're OK with a no op I'll switch it back

@iamkoch

This comment has been minimized.

Show comment
Hide comment
@iamkoch

iamkoch Apr 5, 2016

Member

I've made most of the changes, however I have lost the padding logic that currently resides within property token. I'll look to move that out too.

Member

iamkoch commented Apr 5, 2016

I've made most of the changes, however I have lost the padding logic that currently resides within property token. I'll look to move that out too.

@iamkoch

This comment has been minimized.

Show comment
Hide comment
@iamkoch

iamkoch Apr 5, 2016

Member

I've modified the PR. I'm not entirely sure about the usage of the static method for padding, so I'll await your feedback. I've tried to avoid assignations where possible, understanding performance is preferable to readability, but hopefully what's in place is clear enough.

Member

iamkoch commented Apr 5, 2016

I've modified the PR. I'm not entirely sure about the usage of the static method for padding, so I'll await your feedback. I've tried to avoid assignations where possible, understanding performance is preferable to readability, but hopefully what's in place is clear enough.

/// <summary>
/// Writes the provided value to the output, applying direction-based padding when <paramref name="alignment"/> is provided.
/// </summary>
public static void Apply(TextWriter output, string value, Alignment? alignment)

This comment has been minimized.

@nblumhardt

nblumhardt Apr 5, 2016

Member

Since the method deals with padding for anything and not just Alignment, passing an integer here and avoiding calls when there's no alignment specified seems clearer.

@nblumhardt

nblumhardt Apr 5, 2016

Member

Since the method deals with padding for anything and not just Alignment, passing an integer here and avoiding calls when there's no alignment specified seems clearer.

This comment has been minimized.

@iamkoch

iamkoch Apr 12, 2016

Member

The alignment contains the direction though, which is required in the method body

@iamkoch

iamkoch Apr 12, 2016

Member

The alignment contains the direction though, which is required in the method body

This comment has been minimized.

@nblumhardt

nblumhardt Apr 12, 2016

Member

Ah I see, right you are!

@nblumhardt

nblumhardt Apr 12, 2016

Member

Ah I see, right you are!

@nblumhardt

This comment has been minimized.

Show comment
Hide comment
@nblumhardt

nblumhardt Apr 5, 2016

Member

Looking great. Combed through it pretty closely since it'll be called for pretty much every event that ever passes through Serilog, so please excuse some of the nit-picking :-)

The only other thought left in my mind is whether we should add uppercase and lowercase versions of the short-string table; doing this would avoid all the allocations when ToUpperInvariant()/ToLowerInvariant() are called. May be a "future improvement" to consider.

Member

nblumhardt commented Apr 5, 2016

Looking great. Combed through it pretty closely since it'll be called for pretty much every event that ever passes through Serilog, so please excuse some of the nit-picking :-)

The only other thought left in my mind is whether we should add uppercase and lowercase versions of the short-string table; doing this would avoid all the allocations when ToUpperInvariant()/ToLowerInvariant() are called. May be a "future improvement" to consider.

@iamkoch

This comment has been minimized.

Show comment
Hide comment
@iamkoch

iamkoch Apr 11, 2016

Member

Hi Nick

Just a not to say sorry for the delay in replying - I'll pick up on your comments properly in the next day or two.

Member

iamkoch commented Apr 11, 2016

Hi Nick

Just a not to say sorry for the delay in replying - I'll pick up on your comments properly in the next day or two.

@nblumhardt

This comment has been minimized.

Show comment
Hide comment
@nblumhardt

nblumhardt Apr 11, 2016

Member

@iamkoch no worries! Looking forward to merging this one though, great feature :-)

Member

nblumhardt commented Apr 11, 2016

@iamkoch no worries! Looking forward to merging this one though, great feature :-)

@nblumhardt

This comment has been minimized.

Show comment
Hide comment
@nblumhardt

nblumhardt Apr 19, 2016

Member

How's everything going? Yell out if you're short for time on this, I can also pick up some of the nitty things post-merge if that would help. I appreciate that this one has been a marathon! :-)

Member

nblumhardt commented Apr 19, 2016

How's everything going? Yell out if you're short for time on this, I can also pick up some of the nitty things post-merge if that would help. I appreciate that this one has been a marathon! :-)

@iamkoch

This comment has been minimized.

Show comment
Hide comment
@iamkoch

iamkoch Apr 19, 2016

Member

Hi Nick

Will get this tidied up today. Have pushed a few more things and will fix the dictionary on my lunch break :)

Member

iamkoch commented Apr 19, 2016

Hi Nick

Will get this tidied up today. Have pushed a few more things and will fix the dictionary on my lunch break :)

@iamkoch

This comment has been minimized.

Show comment
Hide comment
@iamkoch

iamkoch Apr 19, 2016

Member

Hi Nick

I've hopefully updated based on all your comments and left the once we've discussed.

Let me know if there's anything left.

The only thing might be the formatting of the multi dimensional array, but I think it's quite readable now, and hopefully you do too! :)

Member

iamkoch commented Apr 19, 2016

Hi Nick

I've hopefully updated based on all your comments and left the once we've discussed.

Let me know if there's anything left.

The only thing might be the formatting of the multi dimensional array, but I think it's quite readable now, and hopefully you do too! :)

@nblumhardt

This comment has been minimized.

Show comment
Hide comment
@nblumhardt

nblumhardt Apr 19, 2016

Member

Awesome! It looks great.

I only have a couple of trivial formatting nits, but I'll spare us another round-trip and apply them on the way in :-)

Member

nblumhardt commented Apr 19, 2016

Awesome! It looks great.

I only have a couple of trivial formatting nits, but I'll spare us another round-trip and apply them on the way in :-)

@nblumhardt nblumhardt merged commit 7fc443e into serilog:dev Apr 19, 2016

1 check passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details

@nblumhardt nblumhardt changed the title from add fixed width functionality for log event output. to Fixed width functionality for log event output. Apr 19, 2016

@nblumhardt nblumhardt changed the title from Fixed width functionality for log event output. to Fixed width functionality for output level names May 12, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment