-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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 WrapLineLayoutRendererWrapper #1400
Added WrapLineLayoutRendererWrapper #1400
Conversation
Nice! Thanks! |
needs some fix for .net35 ;)
|
@304NotModified I'll look into it. I also want to change a bit the MakeChunks method |
See the PR is send to you |
Yes, thanks ! But I changed implementation to a simple StringBuilder. No need to split and join, much simplier :) |
That's indeed also a nice solution, if no unneeded stringbuilder are created :) |
Done, waiting for AppVeyor to finish ! |
Current coverage is 75.49%
@@ master #1400 diff @@
========================================
Files 267 268 +1
Lines 15607 16016 +409
Methods 0 0
Branches 1642 1729 +87
========================================
+ Hits 11732 12091 +359
- Misses 3492 3516 +24
- Partials 383 409 +26
|
A stringbuilder approach is indeed nicer, but also a bit more work too do it right. |
var chunkLength = WrapLine; | ||
var textLength = text.Length; | ||
|
||
var sb = new StringBuilder(textLength); |
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.
building a stringbuilder isn't always needed
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.
indeed. replaced by a string
Why is the string builder totally removed? I think we had a miscommunication! The |
Ahah sorry, I understood to remove it fully :) I put it back |
if (text.Length <= chunkLength) return text; | ||
|
||
// preallocate correct number of chars | ||
var result = new StringBuilder(text.Length + (text.Length / chunkLength) * Environment.NewLine.Length); |
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.
Good one!
Looks really good! This PR is Almost done! Added some minor notes. |
I think I can merge this after we added some docs to the wiki? |
I can write the new page, then you can link it on the https://github.com/NLog/NLog/wiki/Layout-Renderers page ? |
that would be great! |
There you go : https://github.com/NLog/NLog/wiki/WrapLine-layout-renderer I added tests for 2 other syntaxes :
|
I have no idea WHY commit 80f9e7d went without the method names... |
lol that's weird indeed. |
Thanks for this great submit! |
You're welcome ! I'll look if there are others I can help with ! |
This has been released with NLog 4.3.4 :) |
Implementation following discussion on http://stackoverflow.com/questions/36788754/how-can-i-limit-the-length-of-a-line-in-nlog/36789394