feat: implement file renaming functionality#358
Conversation
adonch
commented
Mar 27, 2026
adonch
commented
- Add renameFile function to handle file renaming via command line.
- Validate input paths and check for existence of source and destination.
- Support renaming to a directory by appending the original file name.
- Add renameFile function to handle file renaming via command line. - Validate input paths and check for existence of source and destination. - Support renaming to a directory by appending the original file name.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Great job on implementing the core logic for moving files! Your code is well-structured and handles most of the requirements correctly.
However, I'm requesting changes because a critical edge case is not handled as required. When the destination path ends with a / but the directory does not exist (e.g., non-existent-dir/), your code should throw an error. Currently, it proceeds with the move operation, which leads to incorrect behavior.
To fix this, please add logic to check if the destination path ends with a path separator. If it does, and that directory doesn't exist, you should report an error and exit instead of attempting to rename the file.
✨ 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
| if (fs.existsSync(newPath)) { | ||
| const stats = fs.statSync(newPath); | ||
|
|
||
| if (stats.isDirectory()) { | ||
| finalDestination = path.join(newPath, path.basename(oldPath)); | ||
| } | ||
| } else { | ||
| const parentDir = path.dirname(newPath); | ||
|
|
||
| if (!fs.existsSync(parentDir)) { | ||
| // eslint-disable-next-line no-console | ||
| console.error(`Destination directory does not exist: ${parentDir}`); | ||
|
|
||
| return; | ||
| } | ||
| } |
There was a problem hiding this comment.
This logic does not fully satisfy the requirement that "If a destination contains / in the end it must be considered as a directory." Specifically, when a given destination path ends with a / but does not exist (e.g., non-existent-dir/), your code doesn't treat it as a non-existent directory. Instead of reporting an error about the missing directory, it proceeds to the fs.renameSync call, which then fails with a generic system error.
- Updated error messages for clarity and consistency. - Removed unnecessary comments and streamlined validation checks. - Ensured proper handling of existing paths and directories.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Great work on the previous fixes! The logic for handling destination paths is now correct. However, one critical requirement is not yet met, so I am requesting a change. The application must support moving only files, but the current version also moves directories.
As noted in the comment on src/app.js, please add a check after verifying the source path exists to ensure it is a file. You can use fs.statSync(oldPath).isFile() for this. If the path points to a directory, the program should throw an error. You are very close to completing this!
✨ 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
| if (!fs.existsSync(oldPath)) { | ||
| // eslint-disable-next-line no-console | ||
| console.error(`Source file does not exist: ${oldPath}`); | ||
|
|
||
| return; | ||
| } |
There was a problem hiding this comment.
This block correctly checks if the source path exists. According to the requirements, 'The app must support only moving of files'. Please add a check here to ensure oldPath points to a file and not a directory. You can use fs.statSync() to get the stats of the path.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Excellent work on this pull request! You've successfully addressed the feedback from the previous review by adding a check to ensure the source path is a file. Your code is now robust, handles all specified edge cases correctly, and fully meets the task requirements. I am happy to approve your solution.
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
2 participants
