chore: add test for flattened 'files' option and clean up code

[Krinkle]

• This feature was added in pull fix: Flatten files array. Closes #142 #150 without a test.
  It now has a test.

• The code was applying the flatten operation to all input,
  but should really only be applied to options.files, not to
  this.files from Grunt, which is already processed.

• The code was checking data.files but using this.files.
  This is working correctly, but looks confusing to a reader.
  data.files is the raw input, and this.files is the version
  of that input after processing by Grunt.
  To improve readability, use that same reference for the check
  as well. In order to keep behaviour identical, use the array's
  length property to determine whether it was set. The files
  array is always available from Grunt, and can be safely accessed
  without additional check. This is covered by the karma:config
  test case, which specifies neither options.files nor data.files.

Ref #150.
Closes #236.

Please make sure that your commit messages follow our convention!

[johnjbarton]

I think this would be clearer and simpler as

// three lines of comment
data.files = _.flattenDeep(options.files || []).concat(
  this.files.map(() => {
    return file.src.map((src) => {
    ...

[Krinkle]

I'd like that, and it avoids an extra array and func-apply indirection. However, it doesn't work as-is because while concat() supports an array as parameter, it only unwraps one layer, and we need exactly two layers. So we need the concat.apply() to ensure each array is its own parameter.

Alternatively, while we don't need arbitrary depth on the mapped files, we could go back to passing the entire expression to flattenDeep, by adding two parentheses:

 // three lines of comment
data.files = _.flattenDeep((options.files || []).concat(
  this.files.map(() => {
    return file.src.map((src) => {
    ...
))

Thoughts?

[johnjbarton]

Sorry I don't understand. If we introduce temps:

const optionsFiles = _.flattenDeep(options.files || []);
const gruntishFiles = this.files.map(...);

then

data.files =  [].concat.apply(optionFiles, gruntishFiles);

concatenates optionFiles with each gruntishFiles element.
And here

data.files = optionFiles.concat(gruntishFiles);

concatenates optionFiles with gruntishFiles in one go?

console.log([].concat.apply([1,2], [3,4]));
console.log([1,2].concat([3,4]));

aren't these both [1,2,3,4]?

If not, then I would prefer being more explicit than relying on the different treatment of two array args via apply().

[Krinkle]

With the proposed snippet, the tests fail as follows:

 Invalid pattern [object Object]!
	Object is missing "pattern" property.

This is because data.files will contain nested arrays from the map() inside the map().

console.log([].concat.apply([1,2], [3,4]));
console.log([1,2].concat([3,4]));

aren't these both [1,2,3,4]?

Yes, their outcome happens to be identical, but their logic is not identical. Instead, these two are identical:

[].concat.apply([1,2], [3,4]);
[1,2].concat(3, 4);

Notice that concat() is called with numbers, not arrays. Function#apply takes two parameters, thisValue and argumentsList.

The following are also identical, and matches grunt-karma code:

[].concat.apply([1, 2], [[3],[4]]);
[1,2].concat([3], [4]);
//> [ 1, 2, 3, 4 ]

But, the proposed snippet would do:

[1,2].concat([[3], [4]]);
//> [ 1, 2, [ 3 ], [ 4 ]

[johnjbarton]

Yes, all of which is why I was suggesting some kind of change in the first place. Since this project may have contributions from many different folks with time commitments elsewhere, simpler code is better. Using apply results in the correct behavior but mixing array concat with argument array containing arrays in concat seems too easy to confuse.

But if you want to go ahead its fine, I was only advocating something less terse and special.

[Krinkle]

Right, I'm all for simpler code. The concat() was already there, and while I agree the code should be simple, I'm also new and didn't want to change too much at once.

Having said that, we need something that's simple, but also still works. I'll open a separate PR for that if you don't mind. I've got a few ideas :)