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

Idea: make response.body raise an error when a response_block was used #180

Open
myronmarston opened this issue Jan 18, 2013 · 3 comments
Open
Labels
Milestone

Comments

@myronmarston
Copy link
Contributor

In debugging vcr/vcr#245, I discovered that it is due to a bug in WebMock's excon adapter (reported there as bblimke/webmock#246). WebMock doesn't properly handle the case of a response_block being used; it assumes that response.body will have the response body (which isn't such a crazy assumption, really...), but excon's logic does not build up the body string on the response object when a response_block is used, presumably to avoid bloating memory by building up a string the client doesn't need since it's already handling it in the block.

It's an easy mistake to make, and it'd be nice if Excon surfaced the invalid use of response.body when the response block has been used. What do you think of either raising an error from response.body or returning an object from response.body that raises a clear error if any method is called on it? Either way, it would give the user a clear "you're using a response_block, so response.body isn't set; you shouldn't attempt to use it" message.

@geemus
Copy link
Contributor

geemus commented Jan 18, 2013

@myronmarston - I can certainly see how this would be helpful and avoid confusion. I think that the current behavior of NOT filling in the body is still likely to be the right thing to do (it assumes you already used the body and so don't need it, avoids memory bloat, etc), but it can lead to confusing interactions which your proposal would certainly fix. My preference, I think, would be to have response.body itself return an error in this case. I'll try to find some time to experiment with this in the not-too-distant future. Thanks!

@stale
Copy link

stale bot commented Jul 30, 2018

This issue has been automatically marked stale due to inactivity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the wontfix label Jul 30, 2018
@geemus geemus added pinned and removed wontfix labels Jul 30, 2018
@geemus
Copy link
Contributor

geemus commented Jul 30, 2018

Still seems like it could be worth doing something here, even though we didn't get back to it previously.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants