-
Notifications
You must be signed in to change notification settings - Fork 72
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
Spell check and cleanup #55
Conversation
This reverts commit d2d64b5.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great, thanks! Couple of minor comments 👍
(You can tell I originally wrote those doc comments on a long-haul flight 😆 )
@@ -37,8 +37,7 @@ public EventPropertyTokenRenderer(ConsoleTheme theme, PropertyToken token, IForm | |||
public override void Render(LogEvent logEvent, TextWriter output) | |||
{ | |||
// If a property is missing, don't render anything (message templates render the raw token here). | |||
LogEventPropertyValue propertyValue; | |||
if (!logEvent.Properties.TryGetValue(_token.PropertyName, out propertyValue)) | |||
if (!logEvent.Properties.TryGetValue(_token.PropertyName, out LogEventPropertyValue propertyValue)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
var
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will not compile
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My mistake, it fits.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
@@ -66,8 +66,7 @@ int RenderTextToken(TextToken tt, TextWriter output) | |||
|
|||
int RenderPropertyToken(PropertyToken pt, IReadOnlyDictionary<string, LogEventPropertyValue> properties, TextWriter output) | |||
{ | |||
LogEventPropertyValue propertyValue; | |||
if (!properties.TryGetValue(pt.PropertyName, out propertyValue)) | |||
if (!properties.TryGetValue(pt.PropertyName, out LogEventPropertyValue propertyValue)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
var
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
} | ||
|
||
/// <inheritdoc/> | ||
public IReadOnlyDictionary<ConsoleThemeStyle, SystemConsoleThemeStyle> Styles => _styles; | ||
public IReadOnlyDictionary<ConsoleThemeStyle, SystemConsoleThemeStyle> Styles { get; private set; } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 - if assignment only happens in the constructor, this doesn't need private set
- { get; }
alone will do the job.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
Thanks, looking good 👍 (I personally consider |
I agree. Some StyleCop analyzer rules are controversial. String.Empty is just an obvious expression of intent. |
What issue does this PR address?
Just spell check and cleanup
Does this PR introduce a breaking change?
No