Skip to content

Завдання Move files#323

Open
VladSlob wants to merge 1 commit intomate-academy:masterfrom
VladSlob:master
Open

Завдання Move files#323
VladSlob wants to merge 1 commit intomate-academy:masterfrom
VladSlob:master

Conversation

@VladSlob
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’m requesting changes because several critical, blocking requirements are not met and will cause incorrect CLI behavior and failing tests.

Critical issues to fix:

  • Lines 11-15: Enforce exactly two positional arguments. Use a strict check like if (args.length !== 2) and exit non-zero with a clear usage message. This also prevents silently accepting extra args.
  • Lines 6-10: Validate the source path. Ensure it exists and is a regular file via fs.lstatSync(source).isFile(). If missing or not a file, print a clear error and exit with code 1. The spec forbids moving directories.
  • Lines 17-23: Trailing-slash semantics for destination. If destination ends with / (or path.sep), treat it as a directory intent; the directory must already exist, otherwise error and exit non-zero. Also handle the case where destination exists and is a directory even without a trailing slash by joining path.basename(source).
  • Line 25: Implement cross-device move fallback. Wrap fs.renameSync and on EXDEV perform fs.copyFileSync(src, dest) then fs.unlinkSync(src). Ensure cleanup if the copy fails partway.
  • Lines 26-28: Error handling must set a non-zero exit code. After logging, call process.exit(1) (or rethrow) so failures are detectable by callers/tests.

Minor improvements:

  • Provide consistent, helpful error messages (e.g., Usage: node index <source> <destination>, Source must be a file, Destination directory does not exist).

You’ve got a solid start and readable structure, especially for moving into an existing directory. Addressing the items above will bring the CLI in line with the required behavior and make it robust across edge cases.


✨ 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

Comment on lines +11 to +15
if (args.length < 2) {
console.error('You need fileName and destination');

return;
}

Choose a reason for hiding this comment

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

Argument count enforcement — must accept exactly two positional arguments.

Current code uses if (args.length < 2) and then returns, which allows args.length > 2 silently and exits with code 0. The task requires exactly two positional args (no flags or extras). Change the check to if (args.length !== 2) { console.error('Usage: node index <source> <destination>'); process.exit(1); } so invalid usage returns a non-zero exit code. This satisfies: "Ensure exactly two arguments are provided" and "No additional command-line options or flags are supported." (lines 11-15)

Comment on lines +6 to +10
const moveFiles = () => {
const args = process.argv.slice(2);

const [fileName, newPath] = args;

Choose a reason for hiding this comment

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

Missing source validation — must verify source exists and is a regular file.

You destructure fileName from args but never check that the path exists or is a file. The spec requires that moving directories is not supported and that missing sources produce an error. Add a const stat = fs.lstatSync(fileName) and check stat.isFile(). If it doesn't exist or !isFile, print an error and process.exit(1). This addresses: "The source must be a file; moving directories is not supported". (lines 6-10)

Comment on lines +17 to +23
try {
const isDirectory =
fs.existsSync(newPath) && fs.lstatSync(newPath).isDirectory();

const updatedPath = isDirectory
? path.join(newPath, path.basename(fileName))
: newPath;

Choose a reason for hiding this comment

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

Destination trailing-slash semantics and directory existence check.

Right now isDirectory is computed as fs.existsSync(newPath) && fs.lstatSync(newPath).isDirectory(), so if the user passes a destination with a trailing slash (e.g., dir/) but dir does not exist, code will not treat it as a directory and will attempt to rename to that path instead of throwing an error. The requirement is: a trailing slash must be considered a directory target and must exist. Implement explicit detection of trailing slash (e.g., const wantedAsDir = newPath.endsWith(path.sep) || newPath.endsWith('/')) and if wantedAsDir is true, check existence of the directory and error if missing. Also handle the case where newPath exists and is a directory even without trailing slash. This ensures behavior consistent with examples such as node index file.txt dir/. (lines 17-23)

? path.join(newPath, path.basename(fileName))
: newPath;

fs.renameSync(fileName, updatedPath);

Choose a reason for hiding this comment

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

Single fs.renameSync without EXDEV fallback — implement copy+unlink fallback.

fs.renameSync is fine for same-device moves but throws EXDEV when moving across devices. The task requires that the source be removed after a successful move; to handle cross-device cases, catch the EXDEV error and perform a fallback: fs.copyFileSync(src, dest) followed by fs.unlinkSync(src). Also ensure you clean up a partially copied destination on failure. Without this fallback cross-device moves will fail. (line 25)

Comment on lines +26 to +28
} catch (error) {
console.error('Error occurred:', error);
}

Choose a reason for hiding this comment

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

Error handling: logged errors don't cause non-zero exit.

The catch block only logs the error: console.error('Error occurred:', error); but does not process.exit(1). For a CLI, failing operations should exit with a non-zero code so callers and tests detect failure. After logging, call process.exit(1) (or rethrow) to signal failure. This also helps satisfy tests that expect the program to fail for invalid inputs (e.g., missing source or missing destination directory). (lines 26-28)

Comment on lines +17 to +23
try {
const isDirectory =
fs.existsSync(newPath) && fs.lstatSync(newPath).isDirectory();

const updatedPath = isDirectory
? path.join(newPath, path.basename(fileName))
: newPath;

Choose a reason for hiding this comment

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

Behavior for node index file.txt a ambiguous when a exists vs not — ensure correct resolution.

The task states: if a is an existing directory, move to ./a/file.txt. If a does not exist, rename to ./a. Your current isDirectory logic handles the case when a exists and is a dir, but because you don't explicitly handle trailing-slash intent and lack source checks, edge cases can misbehave. Ensure the code first resolves whether newPath refers to a directory (existing or trailing slash) and then construct target with path.join(dir, path.basename(source)) otherwise treat newPath as a file path. (lines 17-23 and 21-23)

Comment on lines +7 to +15
const args = process.argv.slice(2);

const [fileName, newPath] = args;

if (args.length < 2) {
console.error('You need fileName and destination');

return;
}

Choose a reason for hiding this comment

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

Silent acceptance of extra arguments — must reject extras.

The program destructures only two arguments and ignores the rest. The checker may pass extra args to validate strict parsing. Enforce args.length === 2 and exit with an error when there are more args. This enforces "no additional options or flags" requirement. (lines 7-15)

Comment on lines +11 to +28
if (args.length < 2) {
console.error('You need fileName and destination');

return;
}

try {
const isDirectory =
fs.existsSync(newPath) && fs.lstatSync(newPath).isDirectory();

const updatedPath = isDirectory
? path.join(newPath, path.basename(fileName))
: newPath;

fs.renameSync(fileName, updatedPath);
} catch (error) {
console.error('Error occurred:', error);
}

Choose a reason for hiding this comment

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

Improve error messages and early exits for CLI clarity.

Instead of generic messages like You need fileName and destination, provide usage guidance such as Usage: node index <source> <destination> and ensure each failure path (source missing, source is directory, destination dir does not exist) prints a clear message and exits with process.exit(1). This will make the CLI behavior deterministic and testable. (lines 11-15 and 26-28)

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