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

docs: be specific about the unit of maxBuffer - #8815

Closed
timruffles wants to merge 1 commit into
nodejs:masterfrom
timruffles:patch-1
Closed

docs: be specific about the unit of maxBuffer#8815
timruffles wants to merge 1 commit into
nodejs:masterfrom
timruffles:patch-1

Conversation

@timruffles

Copy link
Copy Markdown

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

@cjihrig

cjihrig commented Dec 3, 2014

Copy link
Copy Markdown

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

a0viedo commented Dec 10, 2014

Copy link
Copy Markdown
Member

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
Copy Markdown
Author

@cjihrig good call, have updated.

@a0viedo will do

@timruffles

Copy link
Copy Markdown
Author

@a0viedo done

@cjihrig

cjihrig commented Dec 29, 2014

Copy link
Copy Markdown

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
Copy Markdown
Author

done

@cjihrig

cjihrig commented Jan 23, 2015

Copy link
Copy Markdown

@geek PTAL

@geek

geek commented Jan 23, 2015

Copy link
Copy Markdown
Member

@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

jasnell commented Aug 15, 2015

Copy link
Copy Markdown
Member

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