feat(pipe): add support for pipes between commands#370
Conversation
src/common.js
Outdated
| var that = new String(str); | ||
| that.to = wrap('to', _to, {idx: 1}); | ||
| that.toEnd = wrap('toEnd', _toEnd, {idx: 1}); | ||
| that.cat = wrap('cat', _cat, {idx: 1}); |
There was a problem hiding this comment.
Can this just be shell.to/cat/toEnd/sed/grep/exec? It seems like repeatedly re-wrapping command isn't performant, and it's not DRY.
There was a problem hiding this comment.
In terms of performance, it actually doesn't hurt much. wrap() is actually very quick, since it just returns another function. But I agree with the DRY part, so this should be fixed now.
cb01a59 to
910dd37
Compare
|
@ariporad this should be a much cleaner implementation now. |
| } | ||
| that.stderr = stderr; | ||
| that.to = function() {wrap('to', _to, {idx: 1}).apply(that.stdout, arguments); return that;}; | ||
| that.toEnd = function() {wrap('toEnd', _toEnd, {idx: 1}).apply(that.stdout, arguments); return that;}; |
There was a problem hiding this comment.
Why are we still re-wraping these?
There was a problem hiding this comment.
I believe these are only wrapped in this spot (they're methods, so they're wrapped here now).
There was a problem hiding this comment.
Is is possible to cache the call to common.wrap?
There was a problem hiding this comment.
Not sure of a good way to do this, because if I move it out of the function, then that isn't defined anymore. So then the code only works for strings, not arrays as well. So I think we might have to leave it as-is. The code should still be very efficient, since wrap is just a closure.
Or we can modify .to() to join this if it's an array, but that would incur a performance hit.
|
One small nit, otherwise looks good. |
|
LGTM! |
feat(pipe): add support for pipes between commands
Fixes #148
Commands can now be piped, of the form:
Commands can send their output to another command in a pipe-like fashion.
sed,grep,cat,exec,to, andtoEndcan appear on the right-hand side of a pipe. Pipes can be chained. You can even pipe into asyncexec().This also improves performance slightly. I got rid of the slow, home-grown
escape()function inside ofsrc/exec.jsand replaced it withJSON.stringify(), which I measured to be slightly faster, and much more reliable (it properly escapes newlines, tabs, etc. as well).I couldn't think of any tests to add for piping to
exec()on Windows (can't think of any native commands that can be on the right-hand side of a pipe). I'm fairly certain this code does work on Windows, I just couldn't think of a use-case. Worst case, we can go back and have someone more Windows-savvy make a PR adding the tests.