Skip to content
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

Implement new JsonSerializerGenerator without document/operation support #416

Merged
merged 3 commits into from
May 25, 2021
Merged

Implement new JsonSerializerGenerator without document/operation support #416

merged 3 commits into from
May 25, 2021

Conversation

jdisanti
Copy link
Collaborator

This is more progress towards #161. This renames the existing JsonSerializerGenerator/JsonParserGenerator classes to be prefixed with Serde- to indicate their implementation detail while keeping them wired up, and then introduces a new JsonSerializerGenerator that will ultimately replace the SerdeJsonSerializerGenerator. So far, this new generator is able to generate payload serialization code that passes the included unit test's model, but it doesn't yet attempt to generate operation or document serialization.

This shouldn't have any impact on the existing SDK since nothing is wired up yet.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@jdisanti jdisanti requested a review from rcoh May 25, 2021 17:12
Copy link
Collaborator

@rcoh rcoh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great! left some comments but feel free to address now or after you make the JsonValue refactoring to the serializer library

val valueExpression: String,
val shape: MemberShape,
/** Whether we're working with a JsonObjectWriter (true) or JsonArrayWriter (false) */
val structMember: Boolean,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this was a little confusing when I was reading the code, might make a sealed class for this which will make the usage:

when(parent) {
  is Json.Object -> ...
  is Json.Array -> ...
}

which I think will be much clearer


/** Generates an expression that serializes the given [inner] expression to the object/array */
fun writeInner(w: RustWriter, jsonType: String, key: String, inner: RustWriter.() -> Unit) {
w.rustInline("$writerName.$jsonType(")
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would probably just use withBlock here:

w.withBlock("$writerName.$jsonType(", ");") {
  if (structMember) {
    w.rust("$key ,")
  }
  inner(this)
}

}
}
is BlobShape -> context.writeInner(this, "string_unchecked", key) {
rustInline("&#T($value)", RuntimeType.Base64Encode(runtimeConfig))
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you're welcome to use rustInline if you prefer, but we run cargo fmt on the output so it doesn't really matter if we write inline or not

serializeUnion(SimpleContext(objectName, inner.valueExpression, target))
}
is DocumentShape -> {
// TODO: Implement document shapes
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would probably just leave this in the TODO or at least print a warning.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The unit test will fail if I leave it as a Kotlin TODO() since I have a Document in the union, which is why I left it as the comment. I would hope our protocol tests would fail if I didn't address this comment :)

}

private fun String.borrow(): String = "&$this"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

up to you, but I personally find it a little clearer with the & in front

namespace test
use aws.protocols#restJson1

union Choice {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd throw a jsonName in here somewhere

}

/** Generates an expression that serializes the given [value] expression to the object/array */
fun writeValue(w: RustWriter, jsonType: String, key: String, value: String) = when (structMember) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd probably make jsonType a sealed class that overrides toString

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can get onboard with a simple enum for this, but at least right now, I don't really need it to be an algebraic data type. Kotlin's sealed classes are tempting to reach for coming from Rust, but they're not anywhere near as ergonomic, unfortunately.

@jdisanti jdisanti merged commit 014d2e0 into smithy-lang:main May 25, 2021
@jdisanti jdisanti deleted the json-ser-no-doc-op branch May 25, 2021 18:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants