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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use reactphp/async instead of clue/reactphp-block #77

Merged
merged 1 commit into from Nov 22, 2022

Conversation

dinooo13
Copy link
Contributor

@dinooo13 dinooo13 commented Sep 30, 2022

Copy link
Collaborator

@SimonFrings SimonFrings left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for opening this up 馃憤

I've added some comments below, can you also add references to your PR message like I did in clue/reactphp-mq#34 so we can keep track of where this is coming from

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
tests/FunctionalClientTest.php Outdated Show resolved Hide resolved
tests/FunctionalClientTest.php Outdated Show resolved Hide resolved
tests/FunctionalClientTest.php Outdated Show resolved Hide resolved
tests/FunctionalClientTest.php Outdated Show resolved Hide resolved
tests/FunctionalClientTest.php Outdated Show resolved Hide resolved
tests/IntegrationTest.php Show resolved Hide resolved
tests/IntegrationTest.php Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@dinooo13
Copy link
Contributor Author

dinooo13 commented Oct 4, 2022

Oh.. it was a bit late when I did this and I used multiple selections to do the edits, now everything should be resolved thanks for the review 馃憤馃徏

@dinooo13 dinooo13 force-pushed the async branch 2 times, most recently from 5c9898c to 065c764 Compare October 4, 2022 19:17
@dinooo13
Copy link
Contributor Author

dinooo13 commented Oct 4, 2022

Added a timeout to the ci.yml because the tests on 8.1+ seemed to run endless.

@SimonFrings
Copy link
Collaborator

I stumbled over this behavior in reactphp/socket#283, some tests got stuck in execution because of other tests that didn't close their servers/connections. Seems like this is the same case here. I would suggest we look into this in a different pull request, afterwards we can come back to this one.

@dinooo13
Copy link
Contributor Author

I will look into your solution and open a new PR to fix this 馃憤馃徏

@SimonFrings
Copy link
Collaborator

@dinooo13 Nice work with #79, now that this one is merged we can rebase these in here changes and get this shipped 馃帀

@dinooo13
Copy link
Contributor Author

Should be ready now :shipit:

Copy link
Collaborator

@SimonFrings SimonFrings left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just two little things I noticed, first the timeout in the ci.yml can be removed as I suggested below and there's a $loop = Loop::get(); inside the last test in tests/IntegrationTest.php that is not needed anymore, lets get rid of that too.

.github/workflows/ci.yml Outdated Show resolved Hide resolved
@dinooo13
Copy link
Contributor Author

Both removed 馃憤馃徏

Copy link
Collaborator

@SimonFrings SimonFrings left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work 馃憤

@SimonFrings SimonFrings requested a review from clue October 19, 2022 12:02
@SimonFrings SimonFrings added this to the v1.4.0 milestone Oct 19, 2022
Copy link
Owner

@clue clue left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dinooo13 Thank you for looking into this, this is a big improvement! 馃憤

I've added a couple of questions / remarks, what do you think about this?

tests/FunctionalClientTest.php Show resolved Hide resolved
tests/IntegrationTest.php Outdated Show resolved Hide resolved
tests/IntegrationTest.php Outdated Show resolved Hide resolved
Copy link
Owner

@clue clue left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We're getting there! Spotted two minor oversights, otherwise LGTM! 馃憤

README.md Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
@dinooo13
Copy link
Contributor Author

Fixed 馃憤馃徏

@dinooo13 dinooo13 requested a review from clue November 21, 2022 20:15
Copy link
Owner

@clue clue left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dinooo13 Thanks for the update, keep it up! :shipit:

@clue clue merged commit f800f3a into clue:master Nov 22, 2022
@dinooo13 dinooo13 deleted the async branch November 22, 2022 13:51
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.

None yet

3 participants