-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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 json mapping extra fields #6009
Conversation
If we do this, I guess |
spec/std/json/mapping_spec.cr
Outdated
@@ -293,6 +293,13 @@ describe "JSON mapping" do | |||
person.other["extra2"].should eq [1, 2, 3] | |||
end | |||
|
|||
it "should pack extra fields" do | |||
person = JSONPersonWithExtra.from_json(%({"name": "John", "age": 30, "extra1" : 1, "extra2" : [1,2,3]})) |
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.
Invalid space after key names (extra1
, extra2
).
I'm not sure if this feature is a good idea. What's the use case? Why do we need this in There are ideas to change the way mappings are defined by using meta attributes (see #3620) which will probably mean removing |
For me, this is often needed, and needed mostly for |
Pardon me insisting, but why? You described how this is used (which is quite obvious) but not a real reason.
Would it perhaps be an option to improve the manual implementation and make it easier to use? |
Sometimes third-party api send such json, and you should process it correctly. I not think that manually parse ~30 fields with JSON::Any would be fun. |
I'm still not convinced this is useful enough. But I don't have to decide that either ;) But I have an idea to solve your problem in a more general and less intrusive way: JSON.mapping(strict: ->(key : String, parser : JSON::PullParser) { @extras[key] = JSON::Any.new(parser) }) This needs only one little change in |
callback is nice, but there is also should be way to extend to_json(js) { |js| @extras.each { |k, v| js.field(key) { v.to_json(js) } } } |
Implementing a custom |
so, should i implement callback? or it not decided. |
The idea of a callback is nice. Maybe If eventually we have meta attributes, it would work the same way. |
But let's first design it: what's the name of the method, etc. |
fixed by serializable |
this is extracted for json from #4411