-
Notifications
You must be signed in to change notification settings - Fork 193
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
Finish the JSON serialization refactor #423
Conversation
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! MapShape.key is a real membershape so you can just treat it as a regular member to avoid the need to special case
writer.rustBlock("") { | ||
write("let mut ret = #T::new();", RustType.HashMap.RuntimeType) | ||
data.members.forEach { (k, v) -> | ||
withBlock("ret.insert(${k.value.dq()}.to_string()$lowercase,", ");") { | ||
withBlock("ret.insert(", ");") { |
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.
you should actually just be able to use renderMember(this, shape.key, k, ctx)
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.
Great suggestion. Fixed.
@@ -321,7 +324,12 @@ class JsonSerializerGenerator(protocolConfig: ProtocolConfig) : StructuredDataSe | |||
val keyName = safeName("key") | |||
val valueName = safeName("value") | |||
rustBlock("for ($keyName, $valueName) in ${context.valueExpression.asRef()}") { | |||
serializeMember(MemberContext.mapMember(context, keyName, valueName)) | |||
val keyTarget = model.expectShape(context.shape.key.target) |
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.
not that it totally matters, but you can probably avoid this by going through the memberSerializer
for key
rather than hard coding it. Then you could even have a blob key or something really crazy and it would work fine.
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.
That sounds ideal, but things are more muddy than that since serializeMember
generates code to exercise the JsonValueWriter, which we definitely wouldn't want for the key. If I split serializeMember
into two pieces: one dealing only in values, and one dealing with JsonValueWriter, then this could work. Not sure if it's worth the effort though.
This finishes the serialization part of #161.
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.