-
Notifications
You must be signed in to change notification settings - Fork 407
Add solution #325
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Add solution #325
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1 +1,82 @@ | ||
| /* eslint-disable no-console */ | ||
| // write code here | ||
| 'use strict'; | ||
|
|
||
| const fs = require('fs/promises'); | ||
| const path = require('path'); | ||
|
|
||
| async function move(oldPath, newPath) { | ||
| try { | ||
| if (!oldPath || !newPath) { | ||
| throw new Error('Exactly two arguments are required'); | ||
| } | ||
|
|
||
| if (oldPath === newPath) { | ||
| return; | ||
| } | ||
|
|
||
| const srcStats = await fs.stat(oldPath); | ||
|
|
||
| if (!srcStats.isFile()) { | ||
| throw new Error('Source is not a file'); | ||
| } | ||
|
Comment on lines
+18
to
+22
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Validation: your source validation (stat + isFile()) is correct. However, if fs.stat(oldPath) throws (source missing), that error must propagate to the CLI wrapper so the process can exit non-zero. After removing the swallow-catch the existing logic will correctly cause a rejection on missing source.
Comment on lines
+18
to
+22
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Source validation is correct — ensure errors are not swallowed. You call |
||
|
|
||
| const trailing = newPath.endsWith(path.sep) || newPath.endsWith('/'); | ||
| let dest; | ||
|
|
||
| if (trailing) { | ||
| const destStats = await fs.stat(newPath); | ||
|
|
||
| if (!destStats.isDirectory()) { | ||
| throw new Error('Destination is not a directory'); | ||
| } | ||
|
|
||
| dest = path.join(newPath, path.basename(oldPath)); | ||
|
Comment on lines
+27
to
+34
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Trailing-slash directory handling is implemented but errors must propagate. The trailing-slash branch (lines 27–34) correctly attempts |
||
| } else { | ||
|
Comment on lines
+24
to
+35
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Trailing-slash handling: you correctly detect trailing slash with newPath.endsWith(path.sep) || newPath.endsWith('/'). When |
||
| try { | ||
| const destStats = await fs.stat(newPath); | ||
|
|
||
| if (destStats.isDirectory()) { | ||
| dest = path.join(newPath, path.basename(oldPath)); | ||
| } else { | ||
| dest = newPath; | ||
| } | ||
|
Comment on lines
+36
to
+43
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Check for existing destination to avoid platform-dependent overwrite. Before performing the rename/copy you should explicitly check whether |
||
| } catch (err) { | ||
| if (err.code === 'ENOENT') { | ||
| const destDir = path.dirname(newPath); | ||
| const dirStats = await fs.stat(destDir); | ||
|
|
||
| if (!dirStats.isDirectory()) { | ||
| throw new Error('Destination directory does not exist'); | ||
| } | ||
|
|
||
| dest = newPath; | ||
|
Comment on lines
+44
to
+53
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Destination parent-directory check is correct but must propagate. When |
||
| } else { | ||
| throw err; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| await fs.rename(oldPath, dest); | ||
| console.log(`Moved ${oldPath} to ${dest}`); | ||
|
Comment on lines
+59
to
+61
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Functional gap: fs.rename is invoked directly. On cross-device moves rename can fail with code === 'EXDEV'. Implement a fallback: on EXDEV do a copy (fs.copyFile or streaming copy) from oldPath to dest, then unlink(oldPath) only after a successful copy. Also ensure you remove any partially copied dest if the copy fails. This is required to match mv behavior across filesystems.
Comment on lines
+59
to
+61
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Overwrite safety: before renaming/copying, check whether the computed There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The success console.log(
Comment on lines
+58
to
+61
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. fs.rename has no EXDEV fallback — add copy+unlink on cross-device errors. You call There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do not log success — tool should be silent on success. Remove the |
||
| } catch (err) { | ||
| console.error('Move error:', err.message); | ||
|
Comment on lines
+62
to
+63
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Because of the catch above, thrown errors (e.g., missing source, missing destination directory) are printed but not propagated. The catch block should either rethrow the error or be removed. Currently this behavior prevents the CLI wrapper's .catch from executing. Let errors propagate so the process can exit 1 when move fails. |
||
| } | ||
|
Comment on lines
+9
to
+64
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Top-level error swallowing in move() — prevents callers from observing failures. You wrap the entire |
||
| } | ||
|
|
||
| if (require.main === module) { | ||
| const args = process.argv.slice(2); | ||
|
|
||
| if (args.length !== 2) { | ||
| console.error('Exactly two arguments are required'); | ||
| } | ||
|
Comment on lines
+70
to
+72
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. When the CLI is invoked with a number of args different than two, the code logs an error but does not stop execution. After the error message the script continues, destructures args and calls move with possibly undefined values. Requirement: the app must be executable via
Comment on lines
+70
to
+72
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. CLI argument enforcement: you log an error when args.length !== 2 but do not stop execution. Per the spec you must enforce exactly two positional arguments and exit non-zero if the count is incorrect. Call process.exit(1) (or return after exiting) here and do not call move when the arguments are invalid.
Comment on lines
+70
to
+72
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. CLI argument validation must exit immediately on invalid usage. At lines 70–72 you log when |
||
|
|
||
| const [oldPath, newPath] = args; | ||
|
|
||
| move(oldPath, newPath).catch((err) => { | ||
| console.error(err); | ||
| process.exit(1); | ||
| }); | ||
|
Comment on lines
+76
to
+79
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The CLI wrapper attaches a .catch to move(...) expecting rejections to be handled, but because move currently swallows errors this catch will rarely fire. Ensure move rejects on error (remove swallowing catch) so this .catch can call process.exit(1) and signal failure to the caller/test harness. |
||
| } | ||
|
|
||
| module.exports = { move }; | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. module.exports is fine — keep move async and rejecting on error for tests.
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Critical: The move function wraps its entire body in a try/catch and the catch only logs the error. This swallows errors and prevents the CLI wrapper from receiving a rejected promise. Remove the top-level try/catch here (or rethrow the error after logging) so callers can handle errors and the process can exit with non-zero. This is required so tests and the CLI detect failures (see checklist items about exiting non-zero on errors).