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.

Sign up for GitHub

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

Jump to bottom

Coloredconsole not compiled regex by default #1171

Merged
merged 7 commits into from
Jan 22, 2016
Merged

Coloredconsole not compiled regex by default #1171

merged 7 commits into from
Jan 22, 2016

Conversation

304NotModified
Copy link
Member

  • Added CompileRegex option, false by default. If false the Regex Cache is used.
  • Some small performance improvements

fixes #764

@304NotModified 304NotModified added enhancement Improvement on existing feature needs documentation on wiki labels Jan 17, 2016
@304NotModified 304NotModified added this to the 4.3 milestone Jan 17, 2016
@codecov-io
Copy link

Current coverage is 73.59%

Merging #1171 into master will increase coverage by +0.16% as of b29e425

@@            master   #1171   diff @@
======================================
  Files          265     265       
  Stmts        14977   15080   +103
  Branches      1643    1656    +13
  Methods          0       0       
======================================
+ Hit          10998   11098   +100
  Partial        418     418       
- Missed        3561    3564     +3

Review entire Coverage Diff as of b29e425

Uncovered Suggestions

  1. +0.09% via ...nternal/AspHelper.cs#206...218
  2. +0.09% via ...nternal/AspHelper.cs#188...200
  3. +0.09% via ...nternal/AspHelper.cs#170...182
  4. See 7 more...

Powered by Codecov. Updated on successful CI builds.

@304NotModified 304NotModified mentioned this pull request Jan 17, 2016
Closed
@304NotModified
Copy link
Member Author

Is someone would review this, that would be great :)

@bhaeussermann
Copy link
Contributor

How about supplementing one of the tests to test with both true and false values for this property?

bhaeussermann
bhaeussermann reviewed Jan 19, 2016
View reviewed changes
src/NLog/Targets/ConsoleWordHighlightingRule.cs
return this.CompiledRegex.Replace(message, new MatchEvaluator(this.MatchEvaluator));
if (CompileRegex)
{
return this.CompiledRegex.Replace(message, this.MatchEvaluator);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps we should check if CompiledRegex is null here and then return just message.

@304NotModified
Copy link
Member Author

Thanks @bhaeussermann !

@304NotModified
Copy link
Member Author

Fixes after review applied!

@bhaeussermann
Copy link
Contributor

It looks good to me! Will you update the Wiki? Then I'll merge.

@304NotModified
Copy link
Member Author

done https://github.com/NLog/NLog/wiki/ColoredConsole-target

304NotModified added a commit that referenced this pull request Jan 22, 2016
@304NotModified
Merge pull request #1171 from NLog/coloredconsole-compiled-regex
6958ac4 
Coloredconsole not compiled regex by default
@304NotModified 304NotModified merged commit 6958ac4 into master Jan 22, 2016
@304NotModified 304NotModified deleted the coloredconsole-compiled-regex branch February 3, 2016 22:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@304NotModified 304NotModified

Labels
console-target enhancement Improvement on existing feature
Projects
None yet
Milestone
4.3
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@304NotModified @codecov-io @bhaeussermann @snakefoot