The Solution#364
Conversation
There was a problem hiding this comment.
Pull request overview
Implements a minimal mv-like CLI for moving/renaming files (per the project README), and updates project tooling dependencies.
Changes:
- Added
move(src, dest)implementation insrc/move.jsand wired it into a CLI entrypoint insrc/app.js. - Bumped
@mate-academy/scriptsfrom^1.8.6to^2.1.3(and updated lockfile). - Added a GitHub Actions workflow template file.
Reviewed changes
Copilot reviewed 4 out of 5 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
src/move.js |
New sync file-move implementation with destination path handling. |
src/app.js |
New CLI argument parsing and error reporting; invokes move(). |
package.json |
Updates @mate-academy/scripts version. |
package-lock.json |
Lockfile update for the scripts version bump. |
.github/workflows/test.yml-template |
Adds a workflow template mirroring the existing CI workflow. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| fs.accessSync(src); | ||
| } catch (error) { | ||
| throw new Error('Source file does not exist'); |
| fs.accessSync(targetDir); | ||
| } catch (error) { | ||
| throw new Error('Destination directory does not exist'); |
| fs.copyFileSync(src, destPath); | ||
| fs.unlinkSync(src); |
| const args = process.argv.slice(2); | ||
|
|
||
| if (args.length !== 2) { | ||
| console.error('Wrong number of arguments'); |
| } | ||
| } | ||
|
|
||
| app(); |
| name: Test | ||
|
|
||
| on: | ||
| pull_request: | ||
| branches: [ master ] |
| - uses: actions/checkout@v2 | ||
| - name: Use Node.js ${{ matrix.node-version }} | ||
| uses: actions/setup-node@v1 | ||
| with: | ||
| node-version: ${{ matrix.node-version }} | ||
| - run: npm install |
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Your implementation correctly handles all core file move scenarios as specified in the requirements. The move function properly detects trailing slashes, validates source file existence, checks destination directory availability, and handles all four cases: simple rename, moving into existing directory, moving with trailing slash, and renaming to non-existent name. The code uses copyFileSync + unlinkSync as required. One minor configuration issue to address: The requirement specifies node index <source> <destination> as the entry point, but your code is in src/app.js. You'll need to either create an index.js at the root level that requires ./src/app, or configure the package.json "main" field to point to src/app.js. The failed check job (Cleanup artifacts) is unrelated to your code—it's a GitHub Actions permission issue with artifact deletion. Your code works correctly, and once you set up the proper entry point configuration, everything should be functional as specified.
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Important
Fix the issues, commit the changes, and then re-request my review using the Re-request review button2 in the Reviewers tab.
Tip
If you have any comments about this review, please leave a message in the Add a comment section below and mention me with the @mateacademy-ai-mentor tag3.
I will read your message during my next review of your pull request. Please do not reply directly to my comments, as that feature is not supported yet.
Caution
Do not close the pull request until the review is complete. Closing it early can lead to unexpected behavior and disrupt the review process.
Footnotes
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
The implementation is working correctly and meets all the requirements. The file move functionality properly handles all four cases: simple rename, moving into existing directory, moving with trailing slash, and renaming to non-existent paths. The entry point is correctly set up, and the code uses copyFileSync + unlinkSync as specified. There's one minor observation: the trailing slash detection in src/move.js checks for both forward slashes and backslashes using ||. While this doesn't cause any test failures, POSIX systems only use forward slashes, so you could consider removing the backslash check for stricter POSIX compliance. However, this is purely optional and doesn't block the solution. All critical functionality is implemented correctly, and the code handles edge cases appropriately with proper error messages. Good work on meeting all the checklist requirements.
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
No description provided.