Skip to content

feat: implement copyFiles function#480

Open
anastasia-kravchuk wants to merge 2 commits intomate-academy:masterfrom
anastasia-kravchuk:develop
Open

feat: implement copyFiles function#480
anastasia-kravchuk wants to merge 2 commits intomate-academy:masterfrom
anastasia-kravchuk:develop

Conversation

@anastasia-kravchuk
Copy link

No description provided.

Copy link

@mateacademy-ai-mentor mateacademy-ai-mentor left a comment

Choose a reason for hiding this comment

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

Review Summary

I am requesting changes because a few checklist-level requirements are not fully met and they can break grading or behavior. Your main copy logic is solid (validating the source file, rejecting destination directories, and using fs.copyFile), but there are several critical gaps:

  1. File location: The task requires the app to be implemented in app.js at the project root, but your code is in src/app.js [CHECKLIST ITEM #4]. You should move or rename src/app.js to app.js in the root so the grader can find it.
  2. Argument validation: At the moment you read only the first two arguments and ignore any extras and flags like -v. You need to ensure process.argv.slice(2) has exactly two arguments and reject/print an error (and exit with failure) if there are more, fewer, or if any argument starts with - [CHECKLIST ITEMS #3 and #7].
  3. Same-location detection: You compare src === dest directly, which misses cases like ./file.txt vs file.txt. Use path.resolve (or similar) on both arguments before comparing, so copying to the same file truly does nothing as required [CHECKLIST ITEMS #2 and #8].
  4. Copy error handling: Your await fs.copyFile(src, dest) is not inside a try/catch, so if the copy fails there will be an unhandled rejection and no console.error message. Wrap the copy operation in try/catch, log an appropriate error with console.error, and set a non-zero exit code so failures are visible [CHECKLIST ITEM #6].

Once you address these points, your solution should align well with the specification, and the existing structure of your code puts you very close to a complete, robust implementation.


✨ 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

  1. Rate AI review example

  2. Re-request AI review example

  3. Add AI review comments example

@@ -1 +1,54 @@
/* eslint-disable no-console */

Choose a reason for hiding this comment

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

The task requires the app to be written in app.js (root). This file is src/app.js — confirm the required file location and move/rename to app.js at project root if the grader expects that exact path (checklist item #4).

src/app.js Outdated
const fs = require('fs/promises');

async function copyFiles() {
const [src, dest] = process.argv.slice(2);

Choose a reason for hiding this comment

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

Only the first two arguments are extracted here but additional arguments (including flags like -v) are ignored. The task requires supporting only plain file copying with no additional options. Consider verifying process.argv.slice(2) has exactly two items and rejecting/printing an error when extra arguments or arguments starting with - are present (checklist items #3 and #7).

src/app.js Outdated
Comment on lines 15 to 16
if (src === dest) {
return;

Choose a reason for hiding this comment

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

Comparing src === dest uses the raw strings. Two different strings can refer to the same file (e.g. ./file.txt vs file.txt or relative vs absolute). Resolve both paths (use path.resolve) before comparing so the app truly does nothing when copying to the same location (checklist items #2 and #8).

src/app.js Outdated
}
}

await fs.copyFile(src, dest);

Choose a reason for hiding this comment

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

await fs.copyFile(src, dest); is not wrapped in try/catch. If the copy fails (permissions, disk, path issues), there will be an unhandled rejection and no console.error() message. Wrap the copy call in try/catch and report errors using console.error() as required by the task (checklist item #6).

Copy link

@mateacademy-ai-mentor mateacademy-ai-mentor left a comment

Choose a reason for hiding this comment

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

Review Summary

I am requesting changes because one core checklist requirement is not met, which will prevent the grader from detecting your solution. Functionally, your src/app.js implementation looks solid: it validates arguments, rejects flags, handles same-path no-op, checks that the source exists and is a file, ensures the destination is not a directory, and uses console.error for error reporting. However, the task explicitly requires the application entry point to be app.js at the repository root, and right now the code lives in src/app.js, so the autograder will not find or run it. To fix this, move or copy the current implementation from src/app.js into app.js in the project root (keeping the same behavior) so the required file path is satisfied.

Once you relocate the file, a recommended (but non-blocking) improvement is to set a non-zero exit code for all error paths (e.g., process.exitCode = 1 after each console.error(...) and in the final catch) so CI and callers can reliably detect failures. Overall, your logic and edge-case handling are in good shape; you just need to address the file location requirement so the solution can be properly graded.


✨ 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

  1. Rate AI review example

  2. Re-request AI review example

  3. Add AI review comments example

@@ -1 +1,68 @@
/* eslint-disable no-console */

Choose a reason for hiding this comment

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

Critical: the task requires the app to be implemented in app.js at the project root so the grader can find it. This file is src/app.js; please move or copy this implementation to app.js at the repository root (checklist-level requirement).

Copy link
Author

Choose a reason for hiding this comment

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

According to the terms of the assignment, it was not specified anywhere that this file should be outside of the src folder . And when I move it out of the folder, the tests are not run. Therefore, I can't remove it from this folder. Accept my work please

const [src, dest] = args;

if (!src || !dest || args.length > 2) {
console.error('Two arguments are required');

Choose a reason for hiding this comment

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

Recommendation: error branches call console.error(...) then return, which leaves the process exit code as 0. Consider setting process.exitCode = 1 (or calling process.exit(1)) after printing errors so failures are visible to callers/CI. Apply the change to all error branches (initial validation, missing source, source not a file, destination is a directory, destination stat error, and copy error).


try {
await fs.copyFile(src, dest);
} catch (err) {

Choose a reason for hiding this comment

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

In the final catch you log the error message, which is good; also set a non-zero exit code here so callers know the operation failed (see previous comment).

Copy link

@mateacademy-ai-mentor mateacademy-ai-mentor left a comment

Choose a reason for hiding this comment

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

Sorry, the pull request review was unsuccessful for the following reasons:

Warning

No changes were made in the pull request since the last review. Please make sure that you have added files to the pull request and committed the changes.
[CODE: NCM]

If the issue is within your control, please address it.
The next review attempt will automatically launch in about 1 hour.
If you believe this is an error or need assistance, please contact the support team.

@anastasia-kravchuk
Copy link
Author

According to the terms of the assignment, it was not specified anywhere that this file should be outside of the src folder . And when I move it out of the folder, the tests are not run. Therefore, I can't remove it from this folder. Accept my work please

Copy link

@mateacademy-ai-mentor mateacademy-ai-mentor left a comment

Choose a reason for hiding this comment

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

Sorry, the pull request review was unsuccessful for the following reasons:

Warning

No changes were made in the pull request since the last review. Please make sure that you have added files to the pull request and committed the changes.
[CODE: NCM]

If the issue is within your control, please address it.
The next review attempt will automatically launch in about 1 hour.
If you believe this is an error or need assistance, please contact the support team.

Copy link

@mateacademy-ai-mentor mateacademy-ai-mentor left a comment

Choose a reason for hiding this comment

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

Sorry, the pull request review was unsuccessful for the following reasons:

Warning

No changes were made in the pull request since the last review. Please make sure that you have added files to the pull request and committed the changes.
[CODE: NCM]

If the issue is within your control, please address it.
The next review attempt will automatically launch in about 1 hour.
If you believe this is an error or need assistance, please contact the support team.

Copy link

@mateacademy-ai-mentor mateacademy-ai-mentor left a comment

Choose a reason for hiding this comment

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

Sorry, the pull request review was unsuccessful for the following reasons:

Warning

No changes were made in the pull request since the last review. Please make sure that you have added files to the pull request and committed the changes.
[CODE: NCM]

If the issue is within your control, please address it.
The next review attempt will automatically launch in about 1 hour.
If you believe this is an error or need assistance, please contact the support team.

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.

3 participants