Skip to content

Conversation

@JordonPhillips
Copy link
Contributor

This adds ShapeSerializers and ShapeDeserializers for HTTP bindings. It's a draft that still needs tests but it should work, more or less.

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 March 4, 2025 18:41
@JordonPhillips
Copy link
Contributor Author

I hate that you can't convert to draft after the fact

@nateprewitt
Copy link
Contributor

You can, the UX is just a little out of the way. Under reviewers press this:
Image of Convert to Draft button

@JordonPhillips JordonPhillips marked this pull request as draft March 4, 2025 18:46
Base automatically changed from power-traits to develop March 4, 2025 18:52
@JordonPhillips JordonPhillips force-pushed the http-schema branch 6 times, most recently from 02c58af to 1b1df12 Compare March 6, 2025 13:47
@JordonPhillips JordonPhillips marked this pull request as ready for review March 6, 2025 13:48
@JordonPhillips JordonPhillips changed the title [DRAFT] Add HTTP Binding serializers and deserializers Add HTTP Binding serializers and deserializers Mar 6, 2025
JordonPhillips and others added 7 commits March 7, 2025 13:15
This adds type guard functions for StreamingBlob and BytesReader
since both are difficult to distinguish due to runtime_checkable
not checking async.
return self.document_value # type: ignore


# TODO: Get all this moved over to the http package
Copy link
Contributor

Choose a reason for hiding this comment

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

Does it need to be? I don't think we need to put traits in their "respective" packages

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd tend to agree with this, I think the other PR we moved everything into the centralized location. I like that unless we have a technical limitation of keeping them together. Is the concern that we may include traits that aren't needed if the smithy-http package isn't present, or are we hitting typing problems?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's more about fighting against package size inflation, though practically for HTTP it's not much of an issue.

@haydenbaker
Copy link
Contributor

haydenbaker commented Mar 7, 2025

Looks like you need to run the gradle build one more time before committing, ClientGenerator needs re-formatting.

@JordonPhillips
Copy link
Contributor Author

Looks like you need to run the gradle build one more time before committing, ClientGenerator needs re-formatting.

This PR doesn't touch ClientGenerator, and running spotless changes nothing

@JordonPhillips JordonPhillips merged commit 578e372 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