Skip to content

Centralize http binding matching and omit empty payloads #503

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

Merged
merged 1 commit into from
Jun 5, 2025

Conversation

JordonPhillips
Copy link
Contributor

This changes the way members are classified to use a separate binding matcher. This lets us use the match statement rather than if-else chains and ensures we're minimizing the number of times we have to iterate over the member list.

This also adds the ability to select whether a payload should be omitted if nothing is bound to it.

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 June 3, 2025 13:44
@JordonPhillips JordonPhillips force-pushed the centralize-http-binding-matching branch from 7f3d7df to fb8963a Compare June 3, 2025 13:55
This changes the way members are classified to use a separate binding
matcher. This lets us use the match statement rather than if-else
chains and ensures we're minimizing the number of times we have to
iterate over the member list.

This also adds the ability to select whether a payload should be
omitted if nothing is bound to it.
Copy link
Contributor

@nateprewitt nateprewitt left a comment

Choose a reason for hiding this comment

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

Looks fine overall, couple questions but nothing blocking.

self.response_status = response_status
found_body = False
found_payload = False
self.bindings = [Binding.BODY] * len(struct.members)
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there we're a reason we're initializing this list here if we're going to swap things out below? Is there a benefit to not just initialize it all at once?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The order of the members in the dictionary isn't the same as the modeled index order, so we can't just append and expect the order to be right. I've got some ideas on how to change schemas so the member order is represented in the dict though. Perhaps I should put that up, it'll reduce the amount of generated code too.

self.has_body = found_body
self.has_payload = found_payload

def should_write_body(self, omit_empty_payload: bool) -> bool:
Copy link
Contributor

Choose a reason for hiding this comment

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

nit; We may want to potentially make this a keyword only arg? Calling should_write_body(True) is kind of confusing when the parameter is indicating the opposite. I see we're aliasing the value to a parameter with the same name though in actual usage.

@JordonPhillips JordonPhillips merged commit 5520941 into develop Jun 5, 2025
3 checks passed
@JordonPhillips JordonPhillips deleted the centralize-http-binding-matching branch June 5, 2025 10:06
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