-
Notifications
You must be signed in to change notification settings - Fork 151
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
Refactor: split methods, docs, error handling, smaller methodes and consistency #176
Conversation
db3e542
to
c960d27
Compare
{ | ||
if (validParameterList != null) | ||
if (validParameterList != null && paramPair.Key != NLogLogger.OriginalFormatPropertyName) |
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.
Looks like it is matching the condition just above
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.
yes indeed, was also thinking to refactor that
(note I only moved the check)
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.
refactored :)
Like this better.
|
checks in method, parameter order
Then we need still the 2nd (duplicate) check ? |
Yes you are right. The code validates that that the parameters comes in the expected order. And if not, then allocates the I guess the code code should be extracted to method called. Little confusing that the validation and the creation of the "valid" parameter list is the same method. Guess I was lazy when I wrote the code. |
string.Concat("EventId", eventIdSeparator, "Id"), | ||
string.Concat("EventId", eventIdSeparator, "Name")); | ||
_eventIdPropertyNames = eventIdPropertyNames; | ||
_eventIdPropertyNames = CreateEventIdPropertyNames(eventIdSeparator); |
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.
@snakefoot did I broke the atomic update? See https://travis-ci.org/NLog/NLog.Extensions.Logging/builds/310224901?utm_source=github_status&utm_medium=notification
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.
ow found it :)
@snakefoot could you please create a PR for this? :) |
…onsistency (#176) * Refactor: split methods * refactor * refactor * proper exception handling * move docs * NotImplementedException -> NotSupportedException * refactor duplicate check * refactor to mulitple methods * CaptureMessageProperties consistent with CaptureEventId checks in method, parameter order * split method * fix nullref * merge 2 ifs * move method * reorder methods
Created #180 for you |
No description provided.