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

BasePHP: Use FS.rename for mv method #445

Merged

Conversation

eliot-akira
Copy link
Collaborator

@eliot-akira eliot-akira commented May 24, 2023

What?

Update BasePHP.mv method to use FS.rename instead of FS.mv which doesn't exist.

Why?

The FS method is called rename, not mv, in both Emscripten and Node.js.

Testing Instructions

  1. Check out the branch.
  2. Run npx nx test php-wasm-node.

@adamziel
Copy link
Collaborator

This is great @eliot-akira, thank you so much for finding and fixing this at the framework level – this benefits every Playground project!

@adamziel
Copy link
Collaborator

Would you also add a unit test to prevent any future regressions? They live in php-wasm/node/src/test/php.spec.ts for now.

@eliot-akira
Copy link
Collaborator Author

Sure, I added the following tests:

  • mv() should move a file
  • mv() should replace target file if it exists
  • mv() should throw a useful error when source file does not exist
  • mv() should throw a useful error when target directory does not exist

@adamziel
Copy link
Collaborator

10/10 💯

@adamziel adamziel merged commit 07700f0 into WordPress:trunk May 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants