Layout processinfo with support for custom Format-string #1752
Conversation
Current coverage is 80% (diff: 100%)@@ master #1752 diff @@
==========================================
Files 274 274
Lines 17422 17424 +2
Methods 2741 2742 +1
Messages 0 0
Branches 1966 1966
==========================================
+ Hits 13974 13992 +18
+ Misses 3005 2987 -18
- Partials 443 445 +2
|
| @@ -81,6 +89,19 @@ protected override void InitializeLayoutRenderer() | |||
| } | |||
|
|
|||
| this.process = Process.GetCurrentProcess(); | |||
|
|
|||
| if (!string.IsNullOrEmpty(Format)) | |||
There was a problem hiding this comment.
what about value.ToStringWithOptionalFormat(...) ? (in class FormatHelper)
|
Good idea! But I think all those reflection isn't needed :) See code comment |
snakefoot
force-pushed
the
processinfoformat
branch
from
3177bc0
to
fdda076
Compare
|
Good catch. I have change the logic to use FormatHelper.ToStringWithOptionalFormat, and it still flies. |
| builder.Append(Convert.ToString(this.propertyInfo.GetValue(this.process, null), formatProvider)); | ||
| var value = this.propertyInfo.GetValue(this.process, null); | ||
| if (value != null && this.Format != null) | ||
| builder.Append(Internal.FormatHelper.ToStringWithOptionalFormat(value, this.Format, formatProvider)); |
There was a problem hiding this comment.
This method already handles null for both. ;)
(and it's an extension method)
So my proposal:
builder.Append(value.ToStringWithOptionalFormat(this.Format, formatProvider));
fdda076
to
470cd42
Compare
|
Yes it looks pretty now :) |
|
\0/ I the next major version of NLog we should look how generic handle this for all layoutrenderers. In all cases there should be a Format I think. |
|
|
|
Merged! |
| @@ -69,6 +69,12 @@ public ProcessInfoLayoutRenderer() | |||
| public ProcessInfoProperty Property { get; set; } | |||
There was a problem hiding this comment.
@snakefoot should this one the defaultParameter? See https://github.com/NLog/NLog/wiki/ProcessInfo-Layout-Renderer
There was a problem hiding this comment.
@304NotModified Think it should just have DefaultValue, and not DefaultParameter. Make PR to remove DefaultParameter?
There was a problem hiding this comment.
yes please :) and add the other as default. Thx in advance!
304NotModified
added
the
documentation done
Makes it possible to make a filename like this:
This change is