Update to latest version of tap and tap-parser

[nelsonic]

This pull request updates devDependencies to the latest version of tap & tap-parser
( as discussed in: #312 )

Had to remove tap.createConsumer in favour of concat-stream which is being used in: test/circular-things.js#L3

Hopefully the changes are self-explanatory and not too much diff.

[ljharb]

please make sure all files end in a trailing newline

[ljharb]

So far this looks good.

[nelsonic]

@ljharb build passing. ready for feedback. (thanks!)

[ljharb]

these might read cleaner if they were arrays with .join('\n')?

[ljharb]

or alternatively, .split('\n') on line 10

[nelsonic]

@ljharb yeah, I saw several approaches in the existing tests:
test/circular-things.js#L11-L27 vs: test/skip.js#L11-L21 vs: stackTrace.js#L29-L51 ... happy to go with either, which is preferable?

[ljharb]

Given those three, I think https://github.com/substack/tape/blob/092344b906cd3774ba1812d2db431e77916c9f19/test/skip.js#L11-L21 is the cleanest

[nelsonic]

Ok, cool I will loop through all the files this evening and change them to match the .join('\n') sample in test/skip.js (thanks again for the feedback!)

[nelsonic]

@ljharb tests updated to match the format in test/skip.js as discussed in. (ready for review. thanks!)

[ljharb]

let's use path.join here - ideally the / is never hardcoded, so things work on Windows (even though that's already the case in master)

[nelsonic]

@ljharb, thanks for the tip on cross-platform compatibility. 👍
do we need to remove all forward slashes?
or is it enough to go from:

var ps = spawn(process.execPath, [ __dirname + '/messages/defaults.js' ]);

To:

var ps = spawn(process.execPath, [ require('path').join( __dirname, 'messages/defaults.js') ]);

From @domenic's Gist: https://gist.github.com/domenic/2790533#paths-and-urls I deduce that this should be enough.

[nelsonic]

And if we change this one instance, should we change all (Five) occurrences:
https://github.com/substack/tape/search?utf8=%E2%9C%93&q=spawn%28process.execPath ? (just say the word and I'll update them all to be consistently windows-friendly)

[ljharb]

That gist uses slashes in path.relative (which also won't work, I think), but no, we'd want path.join(__dirname, 'messages', 'defaults.js') (you can verify because path.join('a', 'b\\c') doesn't result in a/b/c, so you can assume that the reverse wouldn't hold in Windows). Yes, it'd be great to update them all.

[domenic]

The reverse does hold in Windows.

Domenic@Revan MINGW64 ~/Dropbox/Programming/WIP/custom-elements-copier (master)
$ node
> path.join("a", "b/c")
'a\\b\\c'

[ljharb]

Ah, that's fascinating, and good to know. So unix paths get special treatment that windows paths don't?

[domenic]

The Windows OS treats / as \ in cases like this, so Node chooses to normalize since doing so has no effect besides making the string consistent. (POSIX does not act symmetrically.)

[ljharb]

@nelsonic in that case, path.join( __dirname, 'messages/defaults.js') is fine throughout. Thanks!

[nelsonic]

@ljharb updates made as discussed in the in-line comments. #314 (comment) Thanks.

[nelsonic]

@ljharb please let me know if anything else is required. (thanks again!)

[ljharb]

i'd prefer this indentation to be such that if the entire invocation doesn't fit on one line, each argument goes on a line by itself, as does the closing invocation paren.

[nelsonic]

@ljharb like this:

var ps = spawn(
  process.execPath,
  [ require('path').join(__dirname, 'messages', 'defaults.js') ]
);

There does not appear to be a line-length rule/guideline in the project.
in-lining the the code is 96 characters is that acceptable?

var ps = spawn(process.execPath, [ require('path').join(__dirname, 'messages', 'defaults.js') ]);

(I'm used to aiming for max 80 chars per line...)

[ljharb]

The first example is exactly what I had in mind.

Personally I find "max line length" restrictions silly. If your first example is too long for you, you could do:

var path = require('path');
var ps = spawn(
  process.execPath,
  [
    path.join(__dirname, 'messages', 'defaults.js')
  ]
);

(in either case, please move the require to a variable at the top level)

[nelsonic]

Good point!
Went with the single line with the `require('path') moved above its only 89 chars which I think is acceptable:

var ps = spawn(process.execPath, [path.join(__dirname, 'messages', 'defaults.js')]);

[ljharb]

Thanks, I think this looks great, and the work is very appreciated!

If you don't mind, it would be great to rebase this down to remove all the "tidy" kind of commits, so that each commit represents a single conceptual change - if that's too much trouble, it's OK, but it'd be nice :-)

After that I'll merge this!

[nelsonic]

@ljharb see: #318 (is that OK?)

[ljharb]

Thanks, that'll be fine - although I'd have preferred to merge this as-is rather than open a second PR