Skip to content

Conversation

@dpovshed
Copy link
Contributor

@dpovshed dpovshed commented May 7, 2015

I believe the $response parameter should be added to not miss very first chunk of data.

Proposed change makes code to work in the same way as the following line
$response->emit('data', array($bodyChunk, $response));
in the Response::handleData function.

I believe the $response parameter should be added to not miss very first chunk of data.

Proposed change makes code to work in the same way as the following line 
$response->emit('data', array($bodyChunk, $response));
in the Response::handleData function.
@clue
Copy link
Member

clue commented May 7, 2015

LGTM 👍

I believe the $response parameter should be added to not miss very first chunk of data.

Proposed change makes code to work in the same way as the following line
$response->emit('data', array($bodyChunk, $response));
in the Response::handleData function.

I'm tempted to suggest rewording this slightly: We never actually documented the event arguments - this PR makes sure we consistently emit the Response object as second parameter of its data event :)

We might want to update our documentation?

* @event data

@event($bodyChunk, Response $thisResponse)

@WyriHaximus
Copy link
Member

Looks mostly good to be me but love to see update to the docs are suggested by @clue and in the README.md 👍 . And make sure the tests run?: https://travis-ci.org/reactphp/http-client/jobs/61631550#L143-L153

@dpovshed
Copy link
Contributor Author

dpovshed commented May 7, 2015

Thanks guys for the rapid feedback!
This is my first patch proposed to the phpreact library, so I may be no aware about all necessary procedures. I think I can handle the required fixes to: tests, docs, README.

Or someone else is already on a way to adjust remaining stuff?

@WyriHaximus
Copy link
Member

WyriHaximus commented May 7, 2015 via email

@dpovshed
Copy link
Contributor Author

dpovshed commented May 9, 2015

Guys, I hope I took into account all propositions, so now PR includes:

  • the original fix;
  • updated tests;
  • doxygen update proposed by @clue ;
  • updated example in README.txt (tested - it is working);
  • minor typo and punctuation fixes in README.txt

waiting for your feedback! 🎯 ;)

@WyriHaximus
Copy link
Member

LGTM 👍 , the type hint is nice to have but not a blocker for me. When @clue and @cboden don't spot any more things I'll merge and tag it.

@clue
Copy link
Member

clue commented May 13, 2015

Good job @dpovshed, LGTM 👍

@dpovshed
Copy link
Contributor Author

Thanks for the review, :) waiting for confirmation and action from @cboden !

@cboden
Copy link
Member

cboden commented May 14, 2015

LGTM

WyriHaximus added a commit that referenced this pull request May 14, 2015
Small fix to emitter of 'data' event
@WyriHaximus WyriHaximus merged commit e0d9994 into reactphp:master May 14, 2015
@WyriHaximus
Copy link
Member

Thank you for your work, I'll tag it shortly 👍

@WyriHaximus
Copy link
Member

Tagged 0.4.2. Thanks again for you work.

@dpovshed dpovshed deleted the patch-1 branch May 14, 2015 21:53
@dpovshed
Copy link
Contributor Author

Thanks guys for your support! Special to @WyriHaximus for nice blog posts here http://blog.wyrihaximus.net/2015/01/reactphp-introduction/

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants