-
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 option to not render empty literals on nested json objects #1580
Conversation
johnkors
commented
Aug 4, 2016
- Nested objects of null value were rendered as empty json literals. Added option to remove these.
Thanks! We review it! |
@@ -68,6 +69,11 @@ public JsonLayout() | |||
public bool SuppressSpaces { get; set; } | |||
|
|||
/// <summary> | |||
/// Gets or sets the option to render the empty object value {} | |||
/// </summary> | |||
public bool RenderEmptyLiteral { get; set; } |
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.
I was thinking of 'RenderEmptyObject`? What do you think?
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.
Yeah, better naming!
Current coverage is 76% (diff: 100%)@@ master #1580 diff @@
==========================================
Files 270 270
Lines 16318 16325 +7
Methods 2611 2612 +1
Messages 0 0
Branches 1775 1777 +2
==========================================
+ Hits 12432 12440 +8
Misses 3483 3483
+ Partials 403 402 -1
|
sb.Append("}"); | ||
var result = sb.ToString(); | ||
|
||
if (string.IsNullOrEmpty(result.Trim()) && !RenderEmptyObject) |
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.
It's maybe even better to keep a boolean value tracking the "empty state"?
Less String operations are better (e.g. trim and string comparisons)
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.
Ah, okay! Yeah, not used to thinking performance up front :) Thanks. Will push an update.
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.
Cool!
Well pretty important stuff to most of the users ;)
One note, let me know what you think |
perfect! 👍 thanks! |
Added on wiki https://github.com/NLog/NLog/wiki/JsonLayout |
4.3.7 is online: https://www.nuget.org/packages/NLog/4.3.7 |
* Added option to not render empty literals on nested json objects (#1580) * Added option to not render empty objects on nested json objects * Add support for name parameter on (#1578) * Bugfix: Use the culture when rendering the layout (#1556) * Added unit tests * Use the correct culture for rendering the string * fix SL / fix tests * fix culture * split IFormatProvider and CultureInfo * added related test * update/expand tests * fix xplat * Allow overwriting possible nlog configuration file paths (#1469) * Allow overwriting possible nlog configuration file paths * Redesign after review * Added tests * Improvements after review * move statics to XmlLoggingConfiguration * FileTarget: Performance improvement for CleanupInvalidFileNameChars (#1582) * FileTarget: Performance improvement for CleanupInvalidFileNameChars * Added unit test * Update for NLog 4.3.7 * Update README.md [skip ci] * fix unit test (#1589) * Misc. fixes for .NET Core * Remove wrap projects