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

on_stats request option #1202

Merged
merged 5 commits into from
Sep 8, 2015
Merged

on_stats request option #1202

merged 5 commits into from
Sep 8, 2015

Conversation

mtdowling
Copy link
Member

This is WIP commit that adds the on_stats request option.

Closes #1152

@mtdowling mtdowling mentioned this pull request Sep 4, 2015
@GrahamCampbell
Copy link
Member

Looks pretty nice. :)

@mtdowling
Copy link
Member Author

Do you think the TransferStats class actually needs accessor methods, or should it just use public properties?

@GrahamCampbell
Copy link
Member

Private properties with accessors is the best design wise, but public properties might save time. I use public properties more and more as I find it makes things easy to test and fast to write.

@stefanruijsenaars
Copy link
Contributor

Just out of interest, what still needs to happen here?

@mtdowling
Copy link
Member Author

@stefanruijsenaars mostly just tests and docs

@mtdowling mtdowling changed the title WIP work on on_stats on_stats request option Sep 6, 2015
@mtdowling
Copy link
Member Author

This feature is now complete. How does it look from a functionality point of view,@GrahamCampbell / @Crell?

use GuzzleHttp\TransferStats;
use GuzzleHttp\Psr7;

class TransferStatsTest extends \PHPUnit_Framework_TestCase
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just curious: you don't use @coversDefaultClass and @Covers right?

@dawehner
Copy link
Contributor

dawehner commented Sep 7, 2015

The general code looks pretty sane for me

mtdowling added a commit that referenced this pull request Sep 8, 2015
@mtdowling mtdowling merged commit 8f62366 into master Sep 8, 2015
@lussoluca
Copy link

It's possibile to extract that information from a middleware?

@dawehner
Copy link
Contributor

dawehner commented Sep 9, 2015

Can't your middleware manipulate the $options passed into the further down middleware/handler and by that register a callable?

@lussoluca
Copy link

Oh yes, I can do that! Thanks

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.

6 participants