-
Notifications
You must be signed in to change notification settings - Fork 774
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
copy*() case-insensitive paths #568
Conversation
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.
The implementation looks good.
dest = path.join(TEST_DIR, 'srcDir') | ||
|
||
try { | ||
fs.copySync(src, dest) |
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.
What if this function won't throw. The test will pass, as the assertions won't get called, right? Shouldn't we throw any error right after fs.copySync
so we make sure we trigger the catch
?
try {
fs.copySync(src, dest)
throw new Error('fs.copySync didn\'t throw')
} ...
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.
@JPeer264 that's a good point! however, we specifically want to test if copySync
throws that error. So, I wouldn't throw error manually there, but I think the better way to make sure copySync
throws that error would be to assert a boolean after try-catch. I'll apply that there.
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.
Quick review LGTM
squashed this 😁 |
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.
LGTM
Summary: Changes are mostly bug fixes, that shouldn't affect us. From the change log: 7.0.1 / 2018-11-07 ------------------ - Fix `removeSync()` on Windows, in some cases, it would error out with `ENOTEMPTY` ([#646](jprichardson/node-fs-extra#646)) - Document `mode` option for `ensureDir*()` ([#587](jprichardson/node-fs-extra#587)) - Don't include documentation files in npm package tarball ([#642](jprichardson/node-fs-extra#642), [#643](jprichardson/node-fs-extra#643)) 7.0.0 / 2018-07-16 ------------------ - **BREAKING:** Refine `copy*()` handling of symlinks to properly detect symlinks that point to the same file. ([#582](jprichardson/node-fs-extra#582)) - Fix bug with copying write-protected directories ([#600](jprichardson/node-fs-extra#600)) - Universalify `fs.lchmod()` ([#596](jprichardson/node-fs-extra#596)) - Add `engines` field to `package.json` ([#580](jprichardson/node-fs-extra#580)) 6.0.1 / 2018-05-09 ------------------ - Fix `fs.promises` `ExperimentalWarning` on Node v10.1.0 ([#578](jprichardson/node-fs-extra#578)) 6.0.0 / 2018-05-01 ------------------ - Drop support for Node.js versions 4, 5, & 7 ([#564](jprichardson/node-fs-extra#564)) - Rewrite `move` to use `fs.rename` where possible ([#549](jprichardson/node-fs-extra#549)) - Don't convert relative paths to absolute paths for `filter` ([#554](jprichardson/node-fs-extra#554)) - `copy*`'s behavior when `preserveTimestamps` is `false` has been OS-dependent since 5.0.0, but that's now explicitly noted in the docs ([#563](jprichardson/node-fs-extra#563)) - Fix subdirectory detection for `copy*` & `move*` ([#541](jprichardson/node-fs-extra#541)) - Handle case-insensitive paths correctly in `copy*` ([#568](jprichardson/node-fs-extra#568)) Reviewed By: jknoxville Differential Revision: D13023753 fbshipit-source-id: 1ecc6f40be4c8146f92dd29ede846b5ab56765ea
fix #565.
copy
andcopySync
:Check paths stricter before performing copy and also check case-insensitive paths. Added tests as well. Also, found some
copySync
test file was actually callingcopy
😁 (I forgot to change it to sync version after copy-paste it!) Fixed that one as well 😄