Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

Testsuite improvements #471

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

Open
WyriHaximus wants to merge 3 commits into reactphp:1.x
base: 1.x
Choose a base branch
Loading
from WyriHaximus-labs:1.x-testsuite-improvements

Conversation

@WyriHaximus
Copy link
Member

@WyriHaximus WyriHaximus commented Sep 13, 2022

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.
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 ^^

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

Reviewers

@clue clue Awaiting requested review from clue

Assignees

No one assigned

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

AltStyle によって変換されたページ (->オリジナル) /