-
Notifications
You must be signed in to change notification settings - Fork 6
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
Consider switching to plain C# types for the data model #20
Comments
I’d like to think some more about this... |
+1 |
@adamchester still thinking? |
Sorry for the delay... I think this is a great idea and probably the best default, especially if there was still a sensible way to enable snapshot capturing |
👍 yes, snapshotting would still happen the same way as it does now - it's just that the wrapper types would be substituted for their native equivalents. E.g. when an array is logged, we allocate an array within a
|
Since this lib isn't itself a logging library, and so probably doesn't need to buy into a whole immutable data model, it seems like both perf and usability would be improved by adopting
object[]
,Dictionary<string, object>
and plainobject
s, instead of theArrayValue
,StructureValue
andScalarValue
wrappers.I.e., if I have a deserialized JSON object including a template, it's more likely I've deserialized it into plain collection types than the
MessageTemplates
data model, and more familiar/simpler to convert into them if I haven't.Any thoughts on this?
The text was updated successfully, but these errors were encountered: