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

correct :max_body_length documentation and spec #355

Merged
merged 2 commits into from
Sep 26, 2018

Conversation

pedep
Copy link
Contributor

@pedep pedep commented Sep 17, 2018

I'm sorry, but it seems #354's assumed behaviour was wrong.
I assumed the :max_length in body/2 would error, because, what would an incomplete response be worth, but it seems it simply returns the body up until :max_length.

I have corrected the doc and spec to reflect this.
I will submit another PR implementing this behaviour in HTTPoison, since my original intention was to protect against too much data being returned with an error.

Sorry for the mistake @edgurgel

@edgurgel
Copy link
Owner

Should we also include an integration test? We can use http://httparrot.herokuapp.com/stream/5 (we have httparrot running locally for tests) and check if less than 5 lines showed up or something like that?

https://github.com/edgurgel/httpoison/blob/master/test/httpoison_test.exs

@pedep
Copy link
Contributor Author

pedep commented Sep 19, 2018

I'm busy tonight, but i'll see if i can write a quick test tomorrow

@edgurgel
Copy link
Owner

No problem, @pedep ! I will probably have some free time this weekend. I can write the test if you couldn't find time 👍

@pedep
Copy link
Contributor Author

pedep commented Sep 24, 2018

@edgurgel I've dug a bit, and i think this spec explains & covers the expected behavior of our usage of :hackney.body/2

@edgurgel
Copy link
Owner

@pedep, fantastic!

@edgurgel edgurgel merged commit da41878 into edgurgel:master Sep 26, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants