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.

Sign up for GitHub

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

Jump to bottom

Use iter_encoded in calculate_content_length #980

Closed
wants to merge 1 commit into from
Closed

Use iter_encoded in calculate_content_length #980

wants to merge 1 commit into from

Conversation

alexpantyukhin
Copy link
Contributor

@alexpantyukhin alexpantyukhin commented Aug 13, 2016 •
edited
Loading

Fix #705

@alexpantyukhin alexpantyukhin changed the title Response content_length uses encoded self.response Response content_length uses encoded self.response https://github.com/pallets/werkzeug/issues/705 Aug 13, 2016
@alexpantyukhin alexpantyukhin changed the title Response content_length uses encoded self.response https://github.com/pallets/werkzeug/issues/705 Response content_length uses encoded self.response #705 Aug 13, 2016
@alexpantyukhin alexpantyukhin force-pushed the calculate_length_use_encoded_reponse branch from 8a30567 to 17c8eb5 Compare August 13, 2016 15:58
@untitaker
Copy link
Contributor

Thanks, could you remove the issue URL from the title and put fix #705 into the body?

@untitaker untitaker self-assigned this Aug 13, 2016
@alexpantyukhin alexpantyukhin changed the title Response content_length uses encoded self.response #705 fix #705 Aug 13, 2016
@alexpantyukhin
Copy link
Contributor Author

sure! Done.

@alexpantyukhin
Copy link
Contributor Author

alexpantyukhin commented Aug 13, 2016 •
edited
Loading

Actually I don't like the approach. It would be cool to encode response one time and use it for content_length and other places (as @touilleMan mentioned). But it needs much more changes which can bring other errors.

@alexpantyukhin alexpantyukhin changed the title fix #705 Use iter_ #705 Aug 13, 2016
@alexpantyukhin alexpantyukhin changed the title Use iter_ #705 Use iter_encoded in calculate_content_length #705 Aug 13, 2016
@alexpantyukhin alexpantyukhin changed the title Use iter_encoded in calculate_content_length #705 Use iter_encoded in calculate_content_length Aug 13, 2016
@untitaker
Copy link
Contributor

I'd actually welcome a PR that refactors get_data such that it supports a cache like Request.get_data.

@alexpantyukhin alexpantyukhin force-pushed the calculate_length_use_encoded_reponse branch from 17c8eb5 to 464b2b9 Compare August 20, 2016 16:54
@alexpantyukhin
Copy link
Contributor Author

@untitaker I have added a PR which implement caching for the Response's get_data #992 . Also I have modified the current PR and calculate_content_length now uses get_data

@alexpantyukhin
Copy link
Contributor Author

Hi! what about this PR?

@untitaker
Copy link
Contributor

It is dependent on #992 IMO and I still have to review that

@alexpantyukhin
Copy link
Contributor Author

Not sure that these are dependent PR's.
The goal of the current PR is fixing the bug of incorrect calcualting of response. And the #992 PR is just caching. Actually which shouldn't affect on the interface of the get_data.

@untitaker
Copy link
Contributor

Sorry for the late response. I think that calculate_content_length should never drain the data stream such that the user can't get the data anymore, which is why I think we need to land caching before we can do this PR

@untitaker
Copy link
Contributor

Thanks, and sorry for the long wait.

@untitaker untitaker closed this May 5, 2017
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 13, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
No reviews
Assignees

@untitaker untitaker

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Response.calculate_content_length doesn't use encoded self.response
2 participants
@alexpantyukhin @untitaker