-
-
Notifications
You must be signed in to change notification settings - Fork 673
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
Add support for ExpandoObject #948
Conversation
var options = MessagePackSerializerOptions.Standard; | ||
var deserializerOptions = MessagePackSerializerOptions.Standard.WithResolver( | ||
CompositeResolver.Create( | ||
PrimitiveObjectResolver.InstanceWithExpandoObject, | ||
MessagePackSerializerOptions.Standard.Resolver)); |
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.
This is somewhat cumbersome. I'd at least like to have just one set of resolvers used both for serializing and deserializing. And if we could reasonably expose a pre-composed resolver, so much the better. But I'm not sure how to do that, nor how worthwhile the effort is.
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.
Hmmm, do I need to support it?
ExpandoObject is a bad as ArrayList, or even worse than that.
I figured this one would need more discussion. I'm curious what you have against ArrayList (the only thing I can think of is that it isn't a generic type), and how that applies to ExpandoObject is freakin' awesome for folks who just want to deserialize something and call into it as naturally as in Javascript. But only one person has asked for it. I personally would rarely (if ever) use it, but unless it messes up the rest of the library in some way, it seems fine to add, IMHO. |
I have no concern if add only Formatter. I don't like the addition to PrimitiveObjectResolver. However, if you really want to add it, okay to add. |
@neuecc I've reduced the impact of the change on the |
This adopts a slighty new usage pattern that IMO is easier to work with and explain, so that's good. :)
86a9245
to
d05ed55
Compare
@AArnott |
I'm glad you like it. I take it you're ok with the change as-is then? I'll give you a few hours to respond before merging. :) |
Consider/TODO:
Should we enable this only in 'trusted data' mode? Perhaps so, ifNo worries. It's a list of keys, not a hashed key.ExpandoObject
uses a hash algorithm that cannot be made secure.ExpandoObject
implementsIDictionary<string, object>
we could enable more scenarios by usingExpandoObject
instead ofDictionary<string, object>
when deserializing untyped data.Closes #446