search

reactphp / stream

Event-driven readable and writable streams for non-blocking I/O in ReactPHP.
https://reactphp.org/stream/
MIT License
626 stars 62 forks source link

Chore: drop $loop input parameters in favor of Loop::get() #176

Open bpolaszek opened 8 months ago

bpolaszek commented 8 months ago

Following roadmap https://github.com/reactphp/http/issues/517

SimonFrings commented 8 months ago

Hey @bpolaszek, thank you for your pull request, always happy about contributions :+1:

I know the description says "not ready for review", but I'll add my initial thoughts anyway 😄

Changing the minimal supported PHP version to PHP 7.1 is something we want to see for ReactPHP v3, but @WyriHaximus already picked this up in #175. This PR is not far from being merged, so you may have to rebase in here once this goes live. In general, I also think we should use separate PRs for these topics to avoid bloating each pull request in complexity, which also makes them easier to review.

The rest of the changes are heading in a good direction. I think you can also take a look at https://github.com/clue/reactphp-redis/pull/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.

bpolaszek commented 8 months ago

Hi @SimonFrings,

Sorry for the delay ! No worries, I did that mostly for the CI to pass and to avoid the IDE to complain I'm using type hints with PHP < 7. I will rebase once it's merged and erase those changes. Usually I would have based my PR on the WyriHaximus-secret-labs:3.x-raise-minimum-php-version-to-7.1PLUS branch, except I didn't fork from him but from reactphp/stream so I cannot base my work on that branch. Regarding the changes, I just dropped a few commits to be more consistent with your example - don't hesitate to tell me if I missed something.

Thanks! Ben

SimonFrings commented 8 months ago

I will rebase once it's merged and erase those changes.

@bpolaszek Sounds good, we're currently focusing on a v1.10.0 for HTTP which we're planning to release around next week, so @clue and I probably need a week or two before continuing with https://github.com/reactphp/stream/pull/175. Maybe we'll find some time in between, but we want to unblock the HTTP v3 development first which is currently depending on the upcoming v1.10.0 as described in https://github.com/reactphp/http/issues/517.

Regarding the changes, I just dropped a few commits to be more consistent with your example - don't hesitate to tell me if I missed something

Nice, I will have a closer look at your changes later today or in the next days, I will keep you posted :+1: