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

React socket change #51

Closed
wants to merge 5 commits into from

Conversation

aedart
Copy link

@aedart aedart commented May 17, 2017

In regards to #50, the StreamingClientTest fails. Having issues mocking a stream class, because it has been marked as final.

Not that this is my business - but final classes, private methods, and private properties are considered to be (to some degree) anti-patterns. They make it very hard for other developers to reuse existing implementation and just extend the parts that they need changes in.

Alin Eugen Deac added 5 commits May 17, 2017 14:03
Need to be able to test this package on it's own, without requiring some kind of "global" vendor setup.
If this package needs to support php v5.3, then we must lower the phpunit dependency - just to be safe.
@clue
Copy link
Owner

clue commented Jul 6, 2017

Thanks for filing this PR and for your patience! 👍

I've just fixed the (unrelated) build errors via #52 and #53 and would love to get this PR in for the next release. Are you planning to update this or would you rather want me to pick this up instead?

@clue clue added this to the v2.0.0 milestone Jul 6, 2017
@aedart
Copy link
Author

aedart commented Jul 6, 2017

Hi Clue

Sadly, I have too much workload for the next 2-3 weeks. Therefore, if you can use any of the changes that I have made, please do so - in whatever way you see fit.

As mentioned previously, StreamingClientTest fails, but I'm sure that it's a minor thing.

Thanks for your efforts - looking forward to this being released.

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.

2 participants