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

docstring for await_next_request_body #67

Merged
merged 3 commits into from
Apr 2, 2024
Merged

docstring for await_next_request_body #67

merged 3 commits into from
Apr 2, 2024

Conversation

tadad
Copy link
Contributor

@tadad tadad commented Apr 1, 2024

resolves #66

@tadad tadad requested a review from nick1udwig April 1, 2024 17:35
Copy link
Member

@nick1udwig nick1udwig left a comment

Choose a reason for hiding this comment

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

This doesn't tell me, the consumer of the API, that if the next Message is not a Request then it will be dropped.

More generally I support @dr-frmr s recommendation to change this function to await_next_message_body() or the like to avoid the unintuitive behavior of dropping the Message entirely. That solution is better than documenting this because it avoids the situation entirely -- whereas the documentation solution requires that a dev reads and understands this footgun.

@nick1udwig
Copy link
Member

Apologies for the changing target here.

Copy link
Member

@nick1udwig nick1udwig left a comment

Choose a reason for hiding this comment

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

Please address comment, then good to go 👍

We'll have to remember to update any processes/scripts that make use of this function when we bump their process_lib version. I know I've seen it in a number of scripts -- and I use it in kit as well now.

@tadad tadad merged commit ee06628 into main Apr 2, 2024
1 check passed
@tadad tadad deleted the da/docstring branch April 2, 2024 18:31
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.

feature: document unintuitive potential side effect of await_next_request_body()
2 participants