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

Add Adapter::getContents & Adapter::putContents #62

Merged
merged 1 commit into from Apr 10, 2019
Merged

Add Adapter::getContents & Adapter::putContents #62

merged 1 commit into from Apr 10, 2019

Conversation

ghost
Copy link

@ghost ghost commented Feb 3, 2019

This PR adds file read and write optimizations on the adapter level. Essentially it collapses open/read/close and open/write/close calls into one. Child Process is such an adapter which can use this optimization to reduce message passing & latency. Eio simulates the optimization by calling the necessary methods in sequence by itself.

Part of #46 & #46 (comment).

I want to add Eio has a very weird behaviour.

Copy link
Member

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

@CharlotteDunois Thank you very much for this PR!

I agree that these methods make perfect sense from a consumer perspective and I agree that their naming makes sense in regards to the other methods. In the future, I would love to reconsider their naming in order to promote them more prominently :shipit:

src/Eio/Adapter.php Outdated Show resolved Hide resolved
@clue clue added this to the v0.2.0 milestone Feb 3, 2019
@ghost
Copy link
Author

ghost commented Feb 3, 2019

@clue If you have any specific naming in mind, speak your mind. If you can think of better names, it's better to change it now than later.

Copy link
Member

@WyriHaximus WyriHaximus left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@WyriHaximus
Copy link
Member

@clue ping

@ghost
Copy link
Author

ghost commented Mar 7, 2019

@clue Reminder. Also a reminder for buzz.

src/AdapterInterface.php Outdated Show resolved Hide resolved
@ghost
Copy link
Author

ghost commented Apr 9, 2019

@clue @jsor @WyriHaximus I've got a new idea on how to expose "non-standardizable" feature. As such I've updated the PR and would need to be looked at again.

Copy link
Member

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

@CharlotteDunois Thanks for the update, LGTM API-wise! 👍

What do you think about the below documentation suggestions?

src/AdapterInterface.php Outdated Show resolved Hide resolved
src/AdapterInterface.php Outdated Show resolved Hide resolved
src/AdapterInterface.php Outdated Show resolved Hide resolved
@ghost
Copy link
Author

ghost commented Apr 10, 2019

@clue Updated!

Copy link
Member

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

@CharlotteDunois Nice, thank you for the quick update, now let's get this shipped! :shipit: 🎉

@WyriHaximus
Copy link
Member

@CharlotteDunois could you squash both commits together? Will merge afterwards 👍

@WyriHaximus WyriHaximus merged commit 71a6514 into reactphp:master Apr 10, 2019
@ghost ghost deleted the file-put-get-rocket branch April 10, 2019 18:57
@clue clue mentioned this pull request Feb 11, 2020
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