Replace NewLines Layout Renderer Wrapper

[jamesuk189]

Added a layout renderer wrapper to replace newline characters with spaces as per issue #86

[304NotModified]

Thanks!

It seems that to unused using are used?

What's the benefit of the config property?

[jamesuk189]

I thought I had removed the unused usings and that property. It is redundant, I added it before I realised initially.

[304NotModified]

OK. You need to copy the file header of another file to it to fix the failing unit test.

[304NotModified]

I looks nice! short and clean 👍

Maybe we could also make it configurable how to replace the newline? Because I was expecting "" (no chars), but a space is also an option. I propose to add the property: public string replacement. The default for space is then fine.

What do you think?

[jamesuk189]

I like the idea. I was thinking it needed an option to control the separation character/string. I figured having no separator would make it hard to read.

[304NotModified]

hint: the [DefaultValue] is only a marker and won't set the value. (it's a common mistake and therefor I have created an unit test for it.)

Scratch that.

Can you use an auto property and init the properties in the constructor? That's preferred and more consistent with other code in NLog. Thanks!

[304NotModified]

You can ignore the failing unit test. That one is failing really sometimes (so there is really something wrong but not in this code)

[jamesuk189]

Sorry, I really should have checked how the other properties had been implemented.

[304NotModified]

No problem. It's just a matter of style.

[304NotModified]

We will include this in 4.1 :)

One last thing: can you document this on the wiki? (new page)

Maybe also reference from https://github.com/NLog/NLog/wiki/Replace-Layout-Renderer

[jamesuk189]

Added a documentation page to the wiki here: https://github.com/NLog/NLog/wiki/Replace-NewLines-Layout-Renderer

[304NotModified]

OK thanks,

I added also your examples and a 4.1 notice.

But I missed something. How does the test ReplaceNewLineOneLineTest work? And can't we replace with empty string now? It seems that ReplaceNewLineOneLineTest is wrong.

[jamesuk189]

The ReplaceNewLineOneLineTest was added for a check that it wasn't applying when it wasn't meant to.

The default could be replaced with an empty string. My preference is to have a space to help keep it a little bit more readable by default.

[304NotModified]

The default of space is good.

But replace with a empty value is a corner case. We need an unit test for that. Can you add this unit test?

[304NotModified]

Thanks! It has been merged :)