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

Add prop #213

Merged
merged 2 commits into from
Dec 29, 2014
Merged

Add prop #213

merged 2 commits into from
Dec 29, 2014

Conversation

asvetlov
Copy link
Member

I guess to add aiohttp.web.StreamResponse.started property.

It should be False if response has not started and True when response.start() has been called.

The reason is: in custom middleware I want to replace response object for, say, rendering pretty looking page for 500 status code.

But if response has been started (it's response for processing streaming video for example) I cannot do that.

Even catching RuntimeError from .start() call cannot solve it if I want to replace the response, not modify it properties.

Objections?

If not I'll prepare full PR with tests etc.

@fafhrd91
Copy link
Member

that is valid reason and small change. i think PR is too heavy for that kind of things, just make change.

@kxepal
Copy link
Member

kxepal commented Dec 29, 2014

Sounds reasonable for me. +1

@ludovic-gasc
Copy link
Contributor

+1

@asvetlov
Copy link
Member Author

I assume +1's as lgtm. Thanks.

P.S. Pull Requests is the best way even for proposals not ready for merging (if it's pointed directly). You can see proposed changes as diff at least.

@fafhrd91
Copy link
Member

everything should be in moderation

@fafhrd91
Copy link
Member

lgtm

asvetlov added a commit that referenced this pull request Dec 29, 2014
@asvetlov asvetlov merged commit cd0a957 into master Dec 29, 2014
@asvetlov asvetlov deleted the add_response_started_property branch December 29, 2014 20:07
@fafhrd91
Copy link
Member

btw there is better api for start() method. start() method can return different object with write_xxx method.
it is late now, but just as notice.

@asvetlov
Copy link
Member Author

@fafhrd91 I don't follow.
start() should make new ResponseImpl for every call?
That's a mess.

@fafhrd91
Copy link
Member

main problem right now, that we have two very different state encoded in one class.
state1. response object before send headers
state2. response object after send headers

you can not use response as state2 response if it is in state1 and same for opposite. so obvious solution is just split those two states into different objects. but thats probably not best solution for python and in any case we already have stable api.

@asvetlov
Copy link
Member Author

Got it.

On Mon, Dec 29, 2014 at 10:19 PM, Nikolay Kim notifications@github.com
wrote:

main problem right now, that we have two very different state encoded in
one class.
state1. response object before send headers
state2. response object after send headers

you can not use response as state2 response if it is in state1 and same
for opposite. so obvious solution is just split those two states into
different objects. but thats probably not best solution for python and in
any case we already have stable api.


Reply to this email directly or view it on GitHub
#213 (comment).

Thanks,
Andrew Svetlov

@lock
Copy link

lock bot commented Oct 30, 2019

This thread has been automatically locked since there has not been
any recent activity after it was closed. Please open a new issue for
related bugs.

If you feel like there's important points made in this discussion,
please include those exceprts into that new issue.

@lock lock bot added the outdated label Oct 30, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Oct 30, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants