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

Always set content-length #71

Closed
wants to merge 1 commit into from
Closed

Always set content-length #71

wants to merge 1 commit into from

Conversation

SeanTAllen
Copy link
Member

Not setting it makes some clients unhappy.

@SeanTAllen SeanTAllen requested a review from a team February 4, 2024 15:23
@SeanTAllen SeanTAllen added the changelog - fixed Automatically add "Fixed" CHANGELOG entry on merge label Feb 4, 2024
@ponylang-main ponylang-main added the discuss during sync Should be discussed during an upcoming sync label Feb 4, 2024
Not setting it makes some clients unhappy.
@mfelsche
Copy link
Contributor

mfelsche commented Feb 5, 2024

I do understand the reason to come up with this fix, but I would suggest another way to fix it. Either we remove the None part from the function argument, or we add a check in both the .bytes() and .array() method, that adds a content-length of 0 if it is not yet set and if no Transfer-Encoding: chunked header is set. In this case the Content-Length header must be omitted.

@SeanTAllen
Copy link
Member Author

@mfelsche i think that the "send no body" helper methods that work with content length of None (or would work with such a change) are a big boon. For something like a one shot redirect, one shouldn't have to supply bytes. I'm not sure what the best way to accomplish is while playing nice with chunked content. This seems relatively small, do you have time to take over?

@mfelsche
Copy link
Contributor

mfelsche commented Feb 5, 2024

I can take this over and try to ensure that all responses created have a content-length header, unless they are chunked.

@SeanTAllen
Copy link
Member Author

awesome. thanks @mfelsche. i am sure you will do a better job than i would.

@SeanTAllen SeanTAllen removed the discuss during sync Should be discussed during an upcoming sync label Feb 6, 2024
jemc
jemc previously approved these changes Feb 6, 2024
@jemc jemc dismissed their stale review February 6, 2024 19:16

waiting to see what mfelsche comes up with

@SeanTAllen SeanTAllen closed this Feb 10, 2024
@SeanTAllen SeanTAllen deleted the content-length branch February 10, 2024 23:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog - fixed Automatically add "Fixed" CHANGELOG entry on merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants