add ability to override execAsync's maxBuffer default of 20MiB#333
add ability to override execAsync's maxBuffer default of 20MiB#333pieterjandesmedt wants to merge 1 commit intoshelljs:masterfrom
Conversation
pieterjandesmedt
commented
Feb 3, 2016
pieterjandesmedt
commented
- add ability to override execAsync's maxBuffer default of 20MiB
- add maxBuffer override to execSync as well
- added constant DEFAULT_MAXBUFFER_SIZE and made sure maxBuffer is an integer
- fixed indentation and removed delimiter in example
add maxBuffer override to execSync as well added constant DEFAULT_MAXBUFFER_SIZE and made sure maxBuffer is an integer fixed indentation and removed delimiter in example add ability to override execAsync's maxBuffer default of 20MiB
There was a problem hiding this comment.
I don't think we need this line.
There was a problem hiding this comment.
I don't see it... why not? If someone decides to input { maxBuffer: "20000" }, line 26 will make that work. If someone decides to input { maxBuffer: "maximum-buffer-yo" }, line 27 will still make that work by falling back to default. Or am I missing something?
There was a problem hiding this comment.
@pieterjandesmedt I think he meant that "hello" is invalid input, so we don't need to support it. I'm fine with either catching the mistake and defaulting to a value (like you've done), or with crashing. I actually prefer your method.
There was a problem hiding this comment.
@pieterjandesmedt: Yeah, I meant that I don't think we need our API to support someone passing in an invalid value.
But now that I think about it, this line will end up being useful for #244, so forget I ever said anything.
|
@pieterjandesmedt:Thanks so much for the PR, I have a few small comments, otherwise it looks great! |
|
This does not resolve any merge conflicts, so CI won't run. @pieterjandesmedt would you like me to copy your diff to a new PR and resolve the conflicts for you? Also, for future reference, you don't have to make fresh PRs when we ask you to "rebase," you just need to use "git push -f" (new PRs can be confusing) |
|
@pieterjandesmedt: As much as I hate to reject a PR, it looks like this has been fixed by #335, so I have to close this. Sorry. |
3 participants
