Suggested fix for #97, review of implications required #107
Conversation
nilsvu
changed the title
|
This looks like it could break a lot of scripts relying on the current behavior. Could you update this to still prepend the slash but somehow parse the negation beforehand? Ideally with a test? Many thanks! |
With the prepended slash, minimatch will never actually return an object with negate: true, so it's impossible to ignore files in clean operations. Please review this change and consider implications to the remaining codebase - perhaps even consider refactoring to use the glob processing implementation in vinyl-fs, that also gulp.src uses, if possible?
| glob = self.join( '!', glob ); | ||
| } | ||
|
|
||
| var mm = new Minimatch( glob, options ); |
There was a problem hiding this comment.
This should properly handle leading negation characters ! while keeping the behaviour of prepending a slash. However, I don't quite see why prepending a slash is necessary or even useful - it may even lead to problems where the user doesn't expect this behaviour.
I'd also suggest moving to the glob parsing of vinyl-fs, as proposed above, but I don't have enough knowledge of the code to implement this.
|
|
||
| done = suite.done( done ); | ||
|
|
||
| suite.vftp.clean( 'test/clean/**', 'test/fixtures' ) | ||
| suite.vftp.clean( [ 'test/clean/**/*', '!test/clean/dontcleanthis/**/*' ], 'test/fixtures' ) | ||
| .on( 'error', done ) |
There was a problem hiding this comment.
This practice of calling done on error, as adopted in the other tests as well, appears problematic to me. For example the operation may fail to delete a file, because its containing directory has already been deleted. This throws an error which is handled here by calling done, essentially marking the test as successful and bypassing the checks below. Try adding a check assert.ok(false) below - it will still succeed if an error occurs.
|
|
||
| done = suite.done( done ); | ||
|
|
||
| suite.vftp.clean( 'test/clean/**', 'test/fixtures' ) | ||
| suite.vftp.clean( [ 'test/clean/**/*', '!test/clean/dontcleanthis/**/*' ], 'test/fixtures' ) |
There was a problem hiding this comment.
This operation doesn't quite work yet, and the problem may lie with the implementation of the clean function. Skimming its implementation, it appears a directory can be deleted even though one or more of its containing files is excluded from the clean operation. I don't have enough insight into the code to fix this quickly, so could use some help here.
With the prepended slash, minimatch will never actually return an object with
negate: true, so it's impossible to ignore files incleanoperations. Please review this change and consider implications to the remaining codebase - perhaps even consider refactoring to use the glob processing implementation invinyl-fs, that alsogulp.srcuses, if possible?Fixes #97