Skip to content
This repository was archived by the owner on Mar 26, 2018. It is now read-only.

Added options.copy #21

Closed
wants to merge 5 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 18 additions & 0 deletions Gruntfile.js
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,24 @@ module.exports = function (grunt) {
},
src: ['test/fixtures/file.png', 'test/fixtures/another.png'],
dest: 'test/tmp'
},
withCopyTrue: {
options: {
algorithm: 'sha1',
length: 4,
copy: 1
},
src: ['test/tmp/another.png'],
dest: 'test/tmp'
},
withCopyFalse: {
options: {
algorithm: 'sha1',
length: 4,
copy: 0
},
src: ['test/tmp/movedfile.png'],
dest: 'test/tmp/copyfalse'
}
},
simplemocha: {
Expand Down
8 changes: 8 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,14 @@ Default: `8`

The number of characters of the file hash to prefix the file name with.

#### options.copy

Type: `Boolean`
Default: `null`

Flag controlling whether the original file is copied or moved. If not set,
other parameters, such as setting a [destination](#destination) control the behaviour.

### Destination

It will overwrite the `src` files if you don't specify a `dest`:
Expand Down
28 changes: 16 additions & 12 deletions tasks/filerev.js
Original file line number Diff line number Diff line change
Expand Up @@ -10,14 +10,14 @@ module.exports = function (grunt) {
var options = this.options({
encoding: 'utf8',
algorithm: 'md5',
length: 8
length: 8,
copy: null
});
var target = this.target;
var filerev = grunt.filerev || {summary: {}};

eachAsync(this.files, function (el, i, next) {
var move = true;


// If dest is furnished it should indicate a directory
if (el.dest) {
// When globbing is used, el.dest contains basename, we remove it
Expand All @@ -34,8 +34,6 @@ module.exports = function (grunt) {
grunt.log.writeln('Destination dir ' + el.dest + ' does not exists for target ' + target + ': creating');
grunt.file.mkdir(el.dest);
}
// We need to copy file as we now have a dest different from the src
move = false;
}

el.src.forEach(function (file) {
Expand All @@ -46,14 +44,20 @@ module.exports = function (grunt) {
var newName = [path.basename(file, ext), suffix, ext.slice(1)].join('.');
var resultPath;

if (move) {
dirname = path.dirname(file);
resultPath = path.resolve(dirname, newName);
fs.renameSync(file, resultPath);
} else {
dirname = el.dest;
resultPath = path.resolve(dirname, newName);
dirname = el.dest ? el.dest : path.dirname(file);
resultPath = path.resolve(dirname, newName);

if (options.copy === null) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe this should be if (!options.copy) {

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a 3-state option. True, False and null. Perhaps the check should be rearranged so that it explicitly does options.copy === true, options.copy === false and then just use this case for the rest. Since null != undefined

// If options.copy is null, defer to wether or not destination is defined (old behavior)
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mwillerich I think this third case is missing from the tests.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ProLoser I just added 2 more tests for this case

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think all this can be rewritten as

if (options.copy || (options.copy === null && el.dest)) {
  grunt.file.copy(file, resultPath);
}
else {
  fs.renameSync(file, resultPath);
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@eddiemonge yes, makes sense. I've added it to the PR on @ProLoser's fork.

if (el.dest) {
grunt.file.copy(file, resultPath);
} else {
fs.renameSync(file, resultPath);
}
} else if (options.copy) {
grunt.file.copy(file, resultPath);
} else {
fs.renameSync(file, resultPath);
}

filerev.summary[path.normalize(file)] = path.join(dirname, newName);
Expand Down
16 changes: 16 additions & 0 deletions test/filerev_test.js
Original file line number Diff line number Diff line change
Expand Up @@ -26,4 +26,20 @@ describe('filerev', function () {
var revisioned= fs.statSync('test/tmp/expand/file.a0539763.png').size;
assert(revisioned === original);
});

it('should copy the file when copy option is true', function () {
var original = fs.statSync('test/fixtures/another.png').size;
var revisioned= fs.statSync('test/tmp/another.37ba.png').size;
assert(revisioned === original);
var fileExists = fs.existsSync('test/tmp/another.png');
assert(fileExists === true);
});

it('should move the file when copy option is false', function () {
var original = fs.statSync('test/fixtures/movedfile.png').size;
var revisioned= fs.statSync('test/tmp/copyfalse/movedfile.37ba.png').size;
assert(revisioned === original);
var fileExists = fs.existsSync('test/tmp/movedfile.png');
assert(fileExists === false);
});
});
Binary file added test/fixtures/movedfile.png
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.