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

Testsuite improvements #471

Open
wants to merge 3 commits into
base: 1.x
Choose a base branch
from

Conversation

WyriHaximus
Copy link
Member

The changes introduced in this PR simplify the tested classes' creation. This will pave the path for a significantly small change set for #425 and upcoming PR's that will add full HTTP 1.1, and 2 in the future that deal with connection reuse. And as such are bound to introduce changes to the affected internal classes.

On it's own these changes might not look like they bring something new to the project, but they are aimed at our developer experience.

This prepares to keep the change set in reactphp#425 for this file limited to the factory method.
This prepares to keep the change set in reactphp#425 for this file limited to the factory method.
@WyriHaximus WyriHaximus added this to the v1.8.0 milestone Sep 13, 2022
@WyriHaximus WyriHaximus marked this pull request as ready for review September 13, 2022 17:11
@SimonFrings
Copy link
Member

@WyriHaximus This is very similar to #469, I see what you're planning to do with this PR, but I would again ask the question if it would be better to introduce these changes in your upcoming PRs for HTTP 1.1 and 2. There could be the case where you work on your upcoming PRs and recognize, that this is not needed and then reverse these changes again.

I also would argue that these changes make it more difficult to understand the code. The current behavior ($parser = new RequestHeaderParser($clock);) shows exactly what happens at this point, while the new behavior ($parser = $this->createRequestHeaderParser($clock);) would hide this part behind an additional function call. I agree with you that it would be easier from a developer's perspective to just add changes to one function and avoid going over every line of code, but everything comes with a price.

I want to be honest, if I'd see the suggested part inside a project, I would be tempted to file a PR and go the other way around, to reduce the extra function call. 😅

But this is my opinion, happy to hear what you think about my wall of text ^^

@WyriHaximus
Copy link
Member Author

@SimonFrings Alternatively we close this and just keep the wall of text inside #425. It's all about that. This is literally extracted from #425 and if you and @clue don't think it should have been extracted, this comment is a bit weird then as it seems to suggest it should be extracted into a different PR: #425 (comment)

@clue clue removed this from the v1.8.0 milestone Sep 29, 2022
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.

3 participants