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

Pass falsy data to ClientRequest.update_body_from_data handler #143

Merged
merged 1 commit into from
Aug 29, 2014
Merged

Pass falsy data to ClientRequest.update_body_from_data handler #143

merged 1 commit into from
Aug 29, 2014

Conversation

kxepal
Copy link
Member

@kxepal kxepal commented Aug 28, 2014

While logic of processing only "true" values to form request body is ok
for plain raw requests, it causes troubles when you inherit from
ClientRequest to produce requests for some specific content type.
For instance, it's ok to pass "falsy" 0, False, {} data if you're
implementing JsonClientRequest class that automatically encodes data
inside update_body_from_data method.

While logic of processing only "true" values to form request body is ok
for plain raw requests, it causes troubles when you inherit from
ClientRequest to produce requests for some specific content type.
For instance, it's ok to pass "falsy" `0`, `False`, `{}` data if you're
implementing JsonClientRequest class that automatically encodes data
inside update_body_from_data method.
@fafhrd91
Copy link
Member

I think we should use some marker object (empty data) and skip 'data' processing for if default value is specified

@asvetlov
Copy link
Member

Agree with @fafhrd91

@kxepal
Copy link
Member Author

kxepal commented Aug 28, 2014

What's the point for sentinel for no data? And how to handle the data then if it suddenly starts to be None, 0, False, etc? For now, there is no case to handle that and method will fall with meaningless error about non-callable value.

@fafhrd91
Copy link
Member

if data is not provided we do not want to attach default value to request.

@kxepal
Copy link
Member Author

kxepal commented Aug 28, 2014

That's clear. I asked about reason of it in light of the fact that method cannot process same None value - you should overload this method and add own logic about. Also, if you has any wrapper around aiohttp.request, with sentinel you have to inject it into your logic what is bad idea. I just don't see the reason to make breaking change for almost no profit.

@fafhrd91
Copy link
Member

ok, that makes sense.

@fafhrd91
Copy link
Member

+1

kxepal added a commit that referenced this pull request Aug 29, 2014
Pass falsy data to ClientRequest.update_body_from_data handler
@kxepal kxepal merged commit d418ca5 into aio-libs:master Aug 29, 2014
@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.

3 participants