Add dest parameter to filter option in copy and copySync #366
Conversation
| @@ -121,29 +147,61 @@ describe('+ copySync()', () => { | |||
|
|
|||
| assert(!fs.existsSync(path.join(dest, IGNORE)), 'directory was not ignored') | |||
| assert(!fs.existsSync(path.join(dest, IGNORE, 'file')), 'file was not ignored') | |||
| done() | |||
There was a problem hiding this comment.
Revert this change. Using done isn't needed since we are using sync methods here.
| @@ -104,9 +129,10 @@ describe('+ copySync()', () => { | |||
| assert(!fs.existsSync(path.join(destSub, j.toString()))) | |||
| } | |||
| } | |||
| done() | |||
There was a problem hiding this comment.
Revert this change. Using done isn't needed since we are using sync methods here.
lib/copy-sync/copy-sync.js
Outdated
| @@ -35,8 +35,8 @@ function copySync (src, dest, options) { | |||
|
|
|||
| if (options.filter instanceof RegExp) { | |||
| console.warn('Warning: fs-extra: Passing a RegExp filter is deprecated, use a function') | |||
| performCopy = options.filter.test(src) | |||
| } else if (typeof options.filter === 'function') performCopy = options.filter(src) | |||
| performCopy = options.filter.test(src, dest) | |||
There was a problem hiding this comment.
This change doesn't do anything; Regex.prototype.test only accepts one param.
There was a problem hiding this comment.
Very true. I missed it. Thanks @RyanZim. Great that you caught this.
lib/copy/ncp.js
Outdated
| @@ -37,11 +37,11 @@ function ncp (source, dest, options, callback) { | |||
| if (filter) { | |||
| if (filter instanceof RegExp) { | |||
| console.warn('Warning: fs-extra: Passing a RegExp filter is deprecated, use a function') | |||
| if (!filter.test(source)) { | |||
| if (!filter.test(source, dest)) { | |||
There was a problem hiding this comment.
This change doesn't do anything; Regex.prototype.test only accepts one param.
…ve done() from copySync test
|
I fixed them. Please let me know if there is any other issues left. |
| fs.mkdirsSync(dest) | ||
| try { | ||
| fs.copySync(src, dest, filter) | ||
| } catch (err) { | ||
| assert.ifError(err) | ||
| } | ||
| assert(!fs.existsSync(path.join(dest, 'subdir'))) | ||
| done() |
There was a problem hiding this comment.
You still need done when you're using setTimeout (since that makes the code async).
|
Should I merge this? |
|
@manidlou It's up to you. I was leaving it for @jprichardson, since I'm a bit tired today and could have missed something. @jprichardson Perhaps we should formalize some collaborator guidelines sometime? |
| @@ -27,15 +27,15 @@ describe('+ copySync()', () => { | |||
| src = path.join(TEST_DIR, 'src') | |||
| dest = path.join(TEST_DIR, 'dest') | |||
|
|
|||
| fs.mkdirsSync(src) | |||
| fs.mkdirSync(src) | |||
There was a problem hiding this comment.
Why the conversion here to mkdirSync from mkdirsSync?
There was a problem hiding this comment.
I can't speak for @manidlou, but mkdirs technically isn't needed, since we know the parent directory exists.
There was a problem hiding this comment.
@jprichardson Perhaps we should formalize some collaborator guidelines sometime? 👍
Why the conversion here to mkdirSync from mkdirsSync?
As @RyanZim mentioned, I thought we wouldn't need to call mkdirs since we know the parent directory exist. So, I guess I should revert to mkdirsSync. Is that right?
And also sorry that I am still not in harmony with you guys. I try my best to be as helpful as possible.
There was a problem hiding this comment.
And also sorry that I am still not in harmony with you guys. I try my best to be as helpful as possible.
@manidlou you're very helpful, thank you!!
@jprichardson Perhaps we should formalize some collaborator guidelines sometime?
I think for now, any modifications (including my own), barring some crazy necessary hot-fix, require at least one other reviewer?
To be clear, it's okay to accept a self-submitted PR, but it must be for a very good reason. We're all professionals here, so I think this loose guideline should be acceptable for now.
|
Let me know if there is anything else that's needed before I publish 2.1.0. |
|
Sounds good. Just a quick question, for issues like #358 that user doesn't experience the issue anymore, is this a correct action to comment something like "closing the issue" and then closing it? By the way, I close this branch as it is already merged. |
You're doing great.
Generally, if the user can't reproduce, then yes, you would comment & close. #358 is a gray area, since it is a problem on other Node versions. I'm personally not sure what to do here. @jprichardson? |
This resolves #351.
I added dest as an additional parameter to filter option in both
copyandcopySync.