Skip to content

Set 'Transfer-Encoding: chunked' if data is a file with length 0#2896

Merged
sigmavirus24 merged 2 commits intopsf:masterfrom
BraulioVM:master
Dec 2, 2015
Merged

Set 'Transfer-Encoding: chunked' if data is a file with length 0#2896
sigmavirus24 merged 2 commits intopsf:masterfrom
BraulioVM:master

Conversation

@BraulioVM
Copy link
Copy Markdown
Contributor

Fixes #2215

This is the first time I contribute to the project so I am not sure at all I put that code where it belongs. I know I have to add tests for this, but wanted to show you this beforehand.

Did I do this right?

Thank you

@BraulioVM BraulioVM changed the title [WIP] Set 'Transfer-Encoded: chunked' if data is a file with length 0 [WIP] Set 'Transfer-Encoding: chunked' if data is a file with length 0 Nov 23, 2015
@Lukasa
Copy link
Copy Markdown
Member

Lukasa commented Nov 23, 2015

@BraulioVM This is a good first pass! I have a draft of this fix sitting around on my laptop, so when I get home I'll show you how I approached it and let you take the ideas you like best.

@BraulioVM
Copy link
Copy Markdown
Contributor Author

@Lukasa Thanks!

I have just seen #2866, which won't allow me to implement the tests I want, so I might as well think of a solution for that issue.

@Lukasa
Copy link
Copy Markdown
Member

Lukasa commented Nov 24, 2015

So I think this change is not quite right. What we want is a simple change around line 437. Right now we test length for whether it is None: we should just broaden that to say if length. That way, we'll fall through to chunked encoding for zero-length files.

@BraulioVM
Copy link
Copy Markdown
Contributor Author

I see. What I did didn't make any sense because file objects are streams. However, this change won't just affect files, it will affect any stream with length 0. Things will keep working, but I thought we just wanted to set 'Transfer-Encoding: chunked' if data was a file object with super_len(data) == 0.

@Lukasa
Copy link
Copy Markdown
Member

Lukasa commented Nov 24, 2015

@BraulioVM That's actually intentional: sending a chunked zero-length stream is fine, there won't be a problem there. That way, the code is clearer. =)

So, this change looks good to me, but I'll let @sigmavirus24 review it.

@BraulioVM
Copy link
Copy Markdown
Contributor Author

Is there anything else I should do regarding this PR?

@Lukasa
Copy link
Copy Markdown
Member

Lukasa commented Dec 2, 2015

@BraulioVM Nope, just waiting for @sigmavirus24 to get enough time to swing by and take a look at it. =)

@sigmavirus24
Copy link
Copy Markdown
Contributor

Woops! Sorry. I lost track of this with recent goings on IRL. This looks great to me! Thanks @BraulioVM

sigmavirus24 added a commit that referenced this pull request Dec 2, 2015
[WIP] Set 'Transfer-Encoding: chunked' if data is a file with length 0
@sigmavirus24 sigmavirus24 merged commit 40ce366 into psf:master Dec 2, 2015
@sigmavirus24 sigmavirus24 changed the title [WIP] Set 'Transfer-Encoding: chunked' if data is a file with length 0 Set 'Transfer-Encoding: chunked' if data is a file with length 0 Dec 2, 2015
@github-actions github-actions Bot locked as resolved and limited conversation to collaborators Sep 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants