Add "encoding" option to exec #763
Conversation
src/exec.js
Outdated
| var stderr = fs.readFileSync(stderrFile, 'utf8'); | ||
| // fs.readFileSync uses buffer encoding by default, so set | ||
| // the encoding only if opts.encoding wasn't set to 'buffer' | ||
| var encoding = opts.encoding !== 'buffer' ? opts.encoding : null; |
There was a problem hiding this comment.
What happens if we get rid of the ternary and replace with encoding = opts.encoding? It seems weird to specify 'buffer' if what it actually turns into is null
There was a problem hiding this comment.
Perhaps this would be clearer:
var stdout;
var stderr;
if (opts.encoding === 'buffer') {
stdout = fs.readFileSync(stdoutFile);
stderr = fs.readFileSync(stderrFile);
} else {
stdout = fs.readFileSync(stdoutFile, opts.encoding);
stderr = fs.readFileSync(stderrFile, opts.encoding);
}There was a problem hiding this comment.
I think this is the way to go:
var stdout = fs.readFileSync(stdoutFile, opts.encoding);
var stderr = fs.readFileSync(stderrFile, opts.encoding);There was a problem hiding this comment.
That won't work, because the 'buffer' encoding is only valid for the child_process methods:
> fs.readFileSync('./package.json', 'buffer')
Error: Unknown encoding: buffer
at assertEncoding (fs.js:88:11)
at Object.fs.readFileSync (fs.js:394:3)
at repl:1:4
at REPLServer.defaultEval (repl.js:272:27)
at bound (domain.js:287:14)
at REPLServer.runBound [as eval] (domain.js:300:12)
at REPLServer.<anonymous> (repl.js:441:12)
at emitOne (events.js:82:20)
at REPLServer.emit (events.js:169:7)
at REPLServer.Interface._onLine (readline.js:203:10)
If you need to get a buffer out of the fs methods, encoding needs to be null or undefined.
There was a problem hiding this comment.
Oh, ok. Nevermind, your proposal SGTM
test/exec.js
Outdated
| t.falsy(shell.error()); | ||
| t.is(result.code, 0); | ||
| t.truthy(Buffer.isBuffer(result.stdout)); | ||
| t.is(result.stdout.toString(), '1234\n'); |
There was a problem hiding this comment.
Yeah, I will add a stderr check here.
Codecov Report
@@ Coverage Diff @@
## master #763 +/- ##
==========================================
- Coverage 97.38% 97.25% -0.13%
==========================================
Files 33 33
Lines 1222 1240 +18
==========================================
+ Hits 1190 1206 +16
- Misses 32 34 +2
Continue to review full report at Codecov.
|
|
I have tested this with something simple, like |
|
Hey we are not using shelljs anymore for this task, so I dont have anything laying around to verify. |
|
I'm going to go ahead and merge this, since it seems like a useful feature to add. |
Fixes #675 by adding support for the
'encoding'option toexec. It technically supported the'encoding'option before, but the return values ofstdoutandstderrdid not honor the encoding. Now, they will.