Skip to content

Rewrite #97

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

Merged
merged 1 commit into from
Feb 9, 2022
Merged

Rewrite #97

merged 1 commit into from
Feb 9, 2022

Conversation

WyriHaximus
Copy link
Member

@WyriHaximus WyriHaximus commented Oct 6, 2021

This is a full rewrite of this package including the following features/changes:

  • ext-uv adapter
  • child process adapter
  • fallback (blocking) adapter (for native Windows)
  • simplified the internals to remove unnecessary complexity
  • file_get_contents/file_put_contents like API to read/write from/to files
  • start has become an object instead of an array

This PR doesn't include the following features/changes as those will be added in with followup PR's:

  • ext-eio adapter
  • macos support
  • mode support to define permissions when creating files/directories
  • symlinks read/creating/deleting
  • deleting of directories

cc: @SimonFrings

@WyriHaximus WyriHaximus force-pushed the rewrite branch 2 times, most recently from 9e4fc37 to 7be39ff Compare October 14, 2021 07:17
@WyriHaximus WyriHaximus force-pushed the rewrite branch 2 times, most recently from 5e6f62c to 6eb3c14 Compare November 10, 2021 15:56
@WyriHaximus WyriHaximus force-pushed the rewrite branch 5 times, most recently from 7a1615c to e34ce17 Compare February 8, 2022 16:53
@WyriHaximus
Copy link
Member Author

@clue @SimonFrings This PR is now fully ready for review. The green build can be found here. And a readable render of the readme here.

Copy link
Member

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

I looked over everything and focused on writing and syntax because functional adjustments can always be done in following PRs. If the tests are green, then I am happy ^^

Still some work to do but this is definitely a better version than before, nice work @WyriHaximus 👍

@WyriHaximus
Copy link
Member Author

@SimonFrings Thanks! Just updated this PR with your suggestions 👍

This is a full rewrite of this package including the following features/changes:
* ext-uv adapter
* child process adapter
* fallback (blocking) adapter (for native Windows)
* simplified the internals to remove unnecessary complexity
* file_get_contents/file_put_contents like API to read/write from/to files
* start has become an object instead of an array

This PR doesn't include the following features/changes as those will be added in with followup PR's:
* ext-eio adapter
* macos support
* mode support to define permissions when creating files/directories
* symlinks read/creating/deleting
* deleting of directories
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.

@WyriHaximus Let's get this shipped! :shipit:

I agree with everything that's been said, so I only briefly eyeballed this. This may not be perfect (yet), but it's definitely a major improvement over the current experimental filesystem implementation. Let's get this in as-is and build on top of this with future PRs 👍

@clue clue merged commit 5fae489 into reactphp:master Feb 9, 2022
@WyriHaximus WyriHaximus deleted the rewrite branch February 9, 2022 14:39
@WyriHaximus
Copy link
Member Author

@clue 🚢 🇮🇹 !

I agree with everything that's been said, so I only briefly eyeballed this. This may not be perfect (yet), but it's definitely a major improvement over the current experimental filesystem implementation. Let's get this in as-is and build on top of this with future PRs +1

Already on it! #99

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.

5 participants