-
Notifications
You must be signed in to change notification settings - Fork 438
Solution #362
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?
Solution #362
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1 +1,78 @@ | ||
| // write code here | ||
| /* eslint-disable no-console */ | ||
| const path = require('path'); | ||
| const fs = require('fs'); | ||
|
|
||
| const moveFile = () => { | ||
| const [file, destination] = process.argv.slice(2); | ||
|
|
||
| if (!file || !destination) { | ||
| console.error('Usage: node app.js <file> <destination>'); | ||
|
|
||
| return; | ||
| } | ||
|
|
||
| const sourcePath = path.resolve(file); | ||
| const destinationPath = path.resolve(destination); | ||
|
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. 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. 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. You call path.resolve(destination) here before following the requirement to preserve and inspect the raw CLI argument for a trailing separator. Capture the raw argument first (e.g. const rawDestination = process.argv[3];) and check its trailing separator before resolving into destinationPath so resolving can't remove the trailing-separator information. |
||
|
|
||
| if (sourcePath === destinationPath) { | ||
| return; | ||
| } | ||
|
|
||
| let data; | ||
|
|
||
| try { | ||
| data = fs.readFileSync(sourcePath); | ||
| } catch (err) { | ||
| console.error(err.message); | ||
|
|
||
| return; | ||
| } | ||
|
|
||
| try { | ||
| if (destination.endsWith('/')) { | ||
|
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. This check only tests for a POSIX '/' trailing separator. Per the spec you must accept both '/' and the platform separator. Use something like |
||
| if (!fs.existsSync(destination)) { | ||
|
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 treating the destination as a directory you call fs.existsSync(destination) on the raw string. After resolving paths you should perform existence checks on the resolved path (e.g. destinationPath) for consistency. Also see the next comment about error handling when the directory is missing. |
||
| console.error(`Destination directory does not exist: ${destination}`); | ||
|
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 requirements mandate that when the intended destination directory does not exist you must throw an Error with the message |
||
|
|
||
| return; | ||
|
Comment on lines
+33
to
+36
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 intended destination directory doesn't exist you |
||
| } else { | ||
| fs.writeFileSync( | ||
| path.join(destinationPath, path.basename(sourcePath)), | ||
| data, | ||
| ); | ||
| } | ||
| } else if (fs.existsSync(destinationPath)) { | ||
| const stats = fs.statSync(destinationPath); | ||
|
|
||
| if (stats.isDirectory()) { | ||
| fs.writeFileSync( | ||
| path.join(destinationPath, path.basename(sourcePath)), | ||
| data, | ||
| ); | ||
| } else { | ||
| fs.writeFileSync(destinationPath, data); | ||
| } | ||
| } else { | ||
| const destinationDir = path.dirname(destinationPath); | ||
|
|
||
| if (!fs.existsSync(destinationDir)) { | ||
| console.error( | ||
| `Destination directory does not exist: ${destinationDir}`, | ||
|
Comment on lines
+58
to
+59
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. Same problem here: replace the |
||
| ); | ||
|
|
||
| return; | ||
|
Comment on lines
+57
to
+62
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 destination directory is missing you
Comment on lines
+57
to
+62
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. Same issue here: logging and returning when |
||
| } | ||
|
|
||
| fs.writeFileSync(destinationPath, data); | ||
| } | ||
|
|
||
| fs.unlinkSync(sourcePath); | ||
| } catch (err) { | ||
| throw new Error(err.message); | ||
| } | ||
| }; | ||
|
|
||
| moveFile(); | ||
|
|
||
| module.exports = { | ||
| moveFile, | ||
| }; | ||
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.
You destructure the CLI args here into
fileanddestination, but you never keep the originaldestinationstring to check for a trailing slash. The spec requires treating a destination that ends with/as a directory. Store the raw argument (e.g.const [file, rawDestination] = ...) before callingpath.resolveso you can detectrawDestination.endsWith('/'). See the description for this rule.