Skip to content
This repository has been archived by the owner on Apr 22, 2023. It is now read-only.

docs: be specific about the unit of maxBuffer #8815

Closed
wants to merge 1 commit into from

Conversation

timruffles
Copy link

It's nice to be specific about units in docs.

@cjihrig
Copy link

cjihrig commented Dec 3, 2014

I'm in favor of specifying the units. Personally, I think it would be better if the existing sentence:

maxBuffer specifies the largest amount of data (in bytes) allowed on stdout or stderr - if this value is exceeded then the child process is killed.

Was used, possibly with minor edits, as the description of the option. maxBuffer {Number} Max bytes (Default: 200*1024) doesn't explain much, and the actual description of what maxBuffer does is only given for exec(), meaning that readers have to jump around the docs.

@a0viedo
Copy link
Member

a0viedo commented Dec 10, 2014

Hey @timruffles could you re-submit this PR to https://github.com/iojs/io.js? Someone could adopt it for you if you are not interested.

@timruffles
Copy link
Author

@cjihrig good call, have updated.

@a0viedo will do

@timruffles
Copy link
Author

@a0viedo done

@cjihrig
Copy link

cjihrig commented Dec 29, 2014

Please wrap lines at 80 characters. Then, LGTM.

@timruffles timruffles force-pushed the patch-1 branch 2 times, most recently from 998dbfe to cb7cc1e Compare December 29, 2014 19:02
the maxBuffer option was not self-documenting, so document
unit and its effect.
@timruffles
Copy link
Author

done

@cjihrig
Copy link

cjihrig commented Jan 23, 2015

@geek PTAL

@geek
Copy link
Member

geek commented Jan 23, 2015

@timruffles looks good... interested in resubmitting after a merge with the current master? Master has changed since you first submitted and the patch doesn't apply nicely :)

@jasnell
Copy link
Member

jasnell commented Aug 15, 2015

Already landed in nodejs/node. Going to go ahead and close this here. We can cherry pick the io.js commit back to v0.12 if necessary. Thanks for the PR!

@jasnell jasnell closed this Aug 15, 2015
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants