Skip to content

Conversation

@JordonPhillips
Copy link
Contributor

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

@JordonPhillips JordonPhillips requested a review from a team as a code owner February 25, 2025 17:32
connection types are internal, and so are not part of the interface shown
above.)

The `ShapeSerializer`s work in exactly the same way as they do for other use
Copy link
Contributor

Choose a reason for hiding this comment

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

Is the shape serializer responsible for both serializing the shape and encoding the event stream message? In general for event streams, I had thought about these as two separate steps / responsibilities - a shape serializer generally would serialize into an event message and then a separate encoder would handle encoding to the binary wire format (and then a separate signer signs that message and re-encodes it).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is the shape serializer responsible for both serializing the shape and encoding the event stream message?

It can be if that makes the most sense in a given scenario. Generally it would be better to do it that way because then you avoid having to allocate an intermediate representation. The need to sign events makes that impractical, however, because the signer needs to add headers.

Deserialization similarly would be better to stream off the wire, but it runs into another issue - async. ShapeDeserializers are synchronous, so we need to read the whole event and then pass it to the deserializer so that it can all be standard.

I had thought about these as two separate steps / responsibilities

This is some space I think could use a touch up in the implementation. Right now it's a bit of both ways. It works, but could be more efficient.

jonathan343
jonathan343 previously approved these changes Mar 10, 2025
...

async def close(self) -> None:
pass
Copy link
Contributor

@jonathan343 jonathan343 Mar 7, 2025

Choose a reason for hiding this comment

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

Why is pass being used here and not ...? Is this indicating that user's shouldn't need to provide their own implementation?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Correct. IIRC one of the type checkers enforces this sort of behavior.

Copy link
Contributor

Choose a reason for hiding this comment

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

This caught my eye because our implementation in packages/smithy-event-stream/src/smithy_event_stream/aio/interfaces.py uses ... instead of pass.

We should probably keep this consistent, but this is also just a design doc so not a big deal.

Co-authored-by: jonathan343 <43360731+jonathan343@users.noreply.github.com>
@JordonPhillips JordonPhillips merged commit 4579c2b into develop Mar 10, 2025
2 checks passed
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.

3 participants