-
Notifications
You must be signed in to change notification settings - Fork 797
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
Introduce ScalarValue.Null #1774
Conversation
Co-authored-by: Ivan Maximov <sungam3r@yandex.ru>
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.
Nice! Seems like a low-risk win 👍
My only thought was whether there'd be any advantage to keeping the static property entirely internal, but seems reasonable to expose it and encourage similar usage in other places.
true
and false
might also be good candidates for this treatment.
I can add cached common values in another pr. |
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.
LGTM
Co-authored-by: C. Augusto Proiete <augusto@proiete.com>
@@ -409,6 +410,7 @@ namespace Serilog.Events | |||
{ | |||
public SequenceValue(System.Collections.Generic.IEnumerable<Serilog.Events.LogEventPropertyValue> elements) { } | |||
public System.Collections.Generic.IReadOnlyList<Serilog.Events.LogEventPropertyValue> Elements { get; } | |||
public static Serilog.Events.SequenceValue Empty { get; } |
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.
@nblumhardt I pulled changes from dev
to fix approval file after merging #1785.
Saves some allocations.