add input option
#15
add input option
#15
Conversation
|
Lol #16. |
|
Lol, this is scary close: 🐐
// @kevva |
|
This has better tests and account for streaming too so 👍 on this. @jamestalmage, you should add the |
| input.pipe(spawned.stdin); | ||
| } else { | ||
| spawned.stdin.write(input); | ||
| spawned.stdin.end(); |
There was a problem hiding this comment.
You can pass input directly to .end()
|
Re fixture, maybe improve it to be: #!/usr/bin/env node
'use strict';
process.stdin.pipe(process.stdout); |
sindresorhus
changed the title
input option
| @@ -116,6 +134,9 @@ module.exports.spawn = function (cmd, args, opts) { | |||
| }; | |||
|
|
|||
| module.exports.sync = function (cmd, args, opts) { | |||
| if (opts && isStream(opts.input)) { | |||
| throw new TypeError('opts.input can not be a stream in sync mode'); | |||
There was a problem hiding this comment.
throw new TypeError('The `input` option cannot be a stream in sync mode');
| @@ -116,6 +133,9 @@ module.exports.spawn = function (cmd, args, opts) { | |||
| }; | |||
|
|
|||
| module.exports.sync = function (cmd, args, opts) { | |||
| if (opts && isStream(opts.input)) { | |||
| throw new TypeError('The `input` option cannot be a stream in sync mode'); | |||
| } | |||
There was a problem hiding this comment.
Just nitpicking here, but an extra line break here :).
|
Updated. |
| input.pipe(spawned.stdin); | ||
| } else { | ||
| spawned.stdin.end(input); | ||
| } |
There was a problem hiding this comment.
Also nitpicking:
if (isStream(input)) {
input.pipe(spawned.stdin);
return;
}
spawned.stdin.end(input);There was a problem hiding this comment.
Hmm. Is there value to that though? I am all for preferring return over nesting some huge else block, but for a short one liner, I think if/else is more readable.
Depending on what we decide, we could make XO catch this one pretty easy.
There was a problem hiding this comment.
Nah, I guess it's a personal preference. If I can, I skip else and return early instead. To me at least, it's somewhat clearer.
May be some value to consistently return as early as possible too rather than doing things differently depending on how deeply nested your if statements are.
There was a problem hiding this comment.
@kevva Open an issue on https://github.com/sindresorhus/eslint-plugin-xo and we can discuss.
|
Yay. Thanks James! This is going to make my CLI tests a lot nicer. |
Fixes #12.
None of the new tests are setup to run on Windows. Is there a cross platform command I can execute that will work well on Travis and AppVeyor?