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

Chore: Drop $loop usages in favor of Loop::get() #221

Draft
wants to merge 2 commits into
base: 3.x
Choose a base branch
from

Conversation

bpolaszek
Copy link

Still a few tests to fix.

@SimonFrings
Copy link
Member

Hey @bpolaszek, thank you for your pull request, always happy about contributions 👍

As I also wrote in reactphp/stream#176, changing the minimal supported PHP version to PHP 7.1 is something we want to see for ReactPHP v3, but I think we should use separate PRs for these topics to avoid bloating each pull request in complexity, which also makes them easier to review. In my opinion, we don't need the PHP 7.1 changes to remove the optional $loop parameter here.

The rest of the changes are heading in a good direction. I think you can also take a look at clue/reactphp-redis#156 and use this as inspiration (for changes, PR description, commit history, etc.), as we also removed the optional $loop parameter in there. Thanks to our consistency across all our projects, we can apply similar changes in here.

Let's focus on either reactphp/stream#176 or this one here first to get a good feeling on what the PR should look like and then use this knowledge for the other pull requests. This way we can avoid doubling the work 👍

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