add task solution by lublubuterbrodi · Pull Request #343 · mate-academy/node_move-files

lublubuterbrodi · 2026-02-10T16:28:47Z

No description provided.

mateacademy-ai-mentor

Review Summary

I am requesting changes because there are a few critical behavioral gaps compared to the task requirements. First, in src/app.js you currently only check that the source path exists, but you do not verify it is a regular file; the specification says the tool must support moving only files, so you should add something like fs.statSync(sourcePath).isFile() and throw an error if it’s not a file. Second, when the destination directory does not exist you throw new Error('Destination directory does not exist') but immediately catch and only log it, which means callers/tests will see a successful (zero-exit) run instead of a failure; you should either rethrow after logging or set a non-zero process.exitCode so this condition is clearly reported. Third, the spec explicitly uses / as the trailing directory marker, but your check uses path.sep, which will behave differently on Windows; please change the logic to check destination.endsWith('/') (or support both / and path.sep with clear precedence). These three issues directly affect correctness and spec compliance, so they need to be addressed before this can be considered complete. Once you tighten the source-type check, fix the error propagation, and align the trailing-slash detection with the spec, the rest of your move/rename logic looks solid and close to fully compliant.

✨ Please rate this review in the Platform's notification center¹. 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 button² 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 tag³.
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.

mateacademy-ai-mentor · 2026-02-10T16:33:38Z

src/app.js

+  const sourcePath = path.resolve(source);
+
+  if (!fs.existsSync(sourcePath)) {
+    // eslint-disable-next-line no-console
+    console.error('Source file does not exist');
+
+    return;


The code checks existence of the source path but does not ensure the source is a file. The task requires supporting only moving of files (not directories). Consider using fs.statSync(sourcePath).isFile() and report an error if it's not a regular file.

mateacademy-ai-mentor · 2026-02-10T16:33:38Z

src/app.js

+    const destinationIsDir =
+      destinationExists && fs.statSync(destinationPath).isDirectory();
+
+    const endsWithSlash = destination.endsWith(path.sep);


You use path.sep to detect a trailing separator. The specification explicitly refers to / as the trailing directory marker. On some platforms path.sep is \, which will make behavior differ from the spec. Consider checking destination.endsWith('/') (or explicitly accept both but document it).

mateacademy-ai-mentor · 2026-02-10T16:33:38Z

src/app.js

+      if (!destinationExists || !destinationIsDir) {
+        throw new Error('Destination directory does not exist');
+      }
+
+      const fileName = path.basename(sourcePath);
+
+      destinationPath = path.join(destinationPath, fileName);
+    }
+
+    fs.renameSync(sourcePath, destinationPath);
+  } catch (error) {
+    // eslint-disable-next-line no-console
+    console.error(error.message);


When the code throws new Error('Destination directory does not exist') it is caught immediately and only logged. The requirement states an error must be thrown when the destination directory does not exist; swallowing it prevents callers/tests from observing a failure. Either do not catch this error here, rethrow after logging, or set a non-zero process.exitCode so the failing condition is visible to the caller.

mateacademy-ai-mentor

Review Summary

I am requesting changes because there are still two functional issues that prevent the CLI from fully meeting the task requirements. First, all error paths currently just log via fail(...) but allow the process to exit with status code 0 (for example in src/app.js around the final catch and when fail('Source must be a file') is called), which means callers and tests cannot reliably detect failures as required for a CLI tool. Please update fail to set a non‑zero exit status (e.g. process.exitCode = 1) or rethrow the error so that any failure path, including the thrown new Error('Destination directory does not exist'), results in a non‑zero exit code. Second, the trailing-directory detection currently also uses path.sep in addition to '/', but the spec explicitly defines a trailing '/' as the marker for directory semantics; adjust this logic (e.g. if (destination.endsWith('/')) { ... }) so behavior exactly matches the specification. Once these two points are fixed, your implementation of mv-like behavior (rename vs move, enforcing source isFile(), and handling missing destination directories) will align well with the requirements. Overall the structure and logic are solid—focus on exit codes and the trailing-slash rule to finish this off.

✨ Please rate this review in the Platform's notification center¹. 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 button² 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 tag³.
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.

mateacademy-ai-mentor · 2026-02-12T07:12:57Z

src/app.js

+function fail(message) {
+  // eslint-disable-next-line no-console
+  console.error(message);


fail only logs the message. To ensure tests/callers detect failures, set a non-zero exit code here (for example process.exitCode = 1) or call process.exit(1). That will make error cases visible to the process runner.

mateacademy-ai-mentor · 2026-02-12T07:12:57Z

src/app.js

+  try {
+    const sourceStat = fs.statSync(sourcePath);
+
+    if (!sourceStat.isFile()) {
+      fail('Source must be a file');
+
+      return;
+    }


Good: you verify the source is a regular file with isFile() which matches the requirement that the app supports moving only files. When you call fail('Source must be a file') make sure fail causes a non-zero exit (see previous comment).

mateacademy-ai-mentor · 2026-02-12T07:12:57Z

src/app.js

+    const endsWithSlash =
+      destination.endsWith('/') || destination.endsWith(path.sep);


The spec explicitly uses '/' as the trailing-directory marker. Right now you also accept path.sep which can change behavior on Windows; change this to only destination.endsWith('/') (or document and enforce a clear precedence) so behavior matches the task description.

mateacademy-ai-mentor · 2026-02-12T07:12:57Z

src/app.js

+      if (!destinationExists || !destinationIsDir) {
+        throw new Error('Destination directory does not exist');


You throw new Error('Destination directory does not exist') when a trailing-slash destination directory is missing, but that error is caught and only logged later. Ensure this results in a non-zero process exit (either by making fail set process.exitCode = 1 or by rethrowing so the process fails).

mateacademy-ai-mentor · 2026-02-12T07:12:57Z

src/app.js

+  } catch (error) {
+    fail(error.message);


The final catch calls fail(error.message) but doesn't otherwise stop the process. Tie this into the fail change (set a non-zero exit code) or rethrow so the CLI returns a failure status to the caller/test harness.

mateacademy-ai-mentor

Review Summary

I am approving your solution as an educational override so you can keep progressing while you address a few remaining issues.

You did a solid job in src/app.js implementing the mv-like behavior: you correctly ensure the source is a file, use destination.endsWith('/') to detect trailing‑slash directory semantics (matching the requirement that only a final / matters), and handle rename vs. move‑into‑directory cases as specified. The main functional gap is in your fail helper (around the error-handling section of src/app.js): it only sets process.exitCode = 1 when process.env.JEST_WORKER_ID is not set, but the task requires all error paths to result in a non‑zero exit status or a thrown error [CHECKLIST ITEM #12]. A good fix is to always set process.exitCode = 1 and, when under Jest, also throw new Error(message) so tests clearly see the failure.

In .github/workflows/test.yml, the steps: key is correct, but each - uses, - name, and - run line needs to be indented two extra spaces so they are children of steps:; otherwise GitHub Actions will reject the workflow and CI won’t run. Once you fix the workflow indentation and adjust fail so every error path fails with a non‑zero exit, your solution will be in very good shape. Overall, your implementation shows a strong grasp of the CLI requirements—these last tweaks are normal polishing steps as you learn, and you’re on the right track.

✨ Please rate this review in the Platform's notification center¹. I will be grateful for your feedback! ✨

Rate AI review example ↩

mateacademy-ai-mentor · 2026-02-12T07:28:42Z

.github/workflows/test.yml

+      matrix:
+        node-version: [20.x]
+
+    steps:


The steps: key is declared here but the subsequent list items are not nested under it — the - uses, - name, and other - run items must be indented further so they are children of steps:. As written the YAML parser will treat these lines as siblings of steps: and produce a syntax/error or unexpected structure. Indent the list items two more spaces (or ensure their indentation is greater than the steps: key).

mateacademy-ai-mentor · 2026-02-12T07:28:42Z

.github/workflows/test.yml

+    - 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
+    - run: npm test


These list-item lines (checkout, setup-node, npm install/test) are currently aligned with steps:; they should be indented so they belong to the steps sequence. For example:

steps: - 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 - run: npm test

Fixing the indentation will allow the workflow to parse and run.

mateacademy-ai-mentor · 2026-02-12T07:28:42Z

src/app.js

+  if (!process.env.JEST_WORKER_ID) {
+    process.exitCode = 1;
+  }


The fail helper currently sets process.exitCode = 1 only when process.env.JEST_WORKER_ID is not set. The task requires that all error paths result in a non-zero exit status (or rethrow), including when fail('Source must be a file') is called and when destination directory errors are thrown. Remove the Jest-specific guard or ensure tests still observe failure (for example: always set process.exitCode = 1, and if running under Jest, also throw new Error(message) so test runners detect the failure).

mateacademy-ai-mentor · 2026-02-12T07:28:42Z

src/app.js

+    const endsWithSlash = destination.endsWith('/');
+
+    if (destinationIsDir || endsWithSlash) {


Good: trailing-directory detection uses destination.endsWith('/'), which matches the spec that only a trailing '/' should indicate directory semantics. This looks correct and aligns with checklist item #11.

add task solution

be985ce

mateacademy-ai-mentor suggested changes Feb 10, 2026

View reviewed changes

lublubuterbrodi added 3 commits February 10, 2026 18:52

add task solution

89f9a40

add task solution

d7eb5e4

add task solution

4b21ef7

lublubuterbrodi requested a review from mateacademy-ai-mentor February 12, 2026 07:10

mateacademy-ai-mentor suggested changes Feb 12, 2026

View reviewed changes

add task solution

8783962

lublubuterbrodi requested a review from mateacademy-ai-mentor February 12, 2026 07:26

mateacademy-ai-mentor approved these changes Feb 12, 2026

View reviewed changes

		const endsWithSlash =
		destination.endsWith('/') \|\| destination.endsWith(path.sep);

		if (!destinationExists \|\| !destinationIsDir) {
		throw new Error('Destination directory does not exist');

		const endsWithSlash = destination.endsWith('/');

		if (destinationIsDir \|\| endsWithSlash) {

Conversation

lublubuterbrodi commented Feb 10, 2026

Uh oh!

mateacademy-ai-mentor left a comment

Choose a reason for hiding this comment

Review Summary

Footnotes

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mateacademy-ai-mentor left a comment

Choose a reason for hiding this comment

Review Summary

Footnotes

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mateacademy-ai-mentor left a comment

Choose a reason for hiding this comment

Review Summary

Footnotes

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants