Skip to content

doc: clarification for maxBuffer and Unicode output#6030

Closed
jasnell wants to merge 1 commit intonodejs:masterfrom
jasnell:child_process_max_buffer
Closed

doc: clarification for maxBuffer and Unicode output#6030
jasnell wants to merge 1 commit intonodejs:masterfrom
jasnell:child_process_max_buffer

Conversation

@jasnell
Copy link
Member

@jasnell jasnell commented Apr 3, 2016

Pull Request check-list

  • Does make -j8 test (UNIX) or vcbuild test nosign (Windows) pass with
    this change (including linting)?
  • Is the commit message formatted according to [CONTRIBUTING.md][0]?
  • If this change fixes a bug (or a performance problem), is a regression
    test (or a benchmark) included?
  • Is a documentation update included (if this change modifies
    existing APIs, or introduces new ones)?

Affected core subsystem(s)

doc

Description of change

Clarify caveats on maxBuffer with regards to Unicode output.

Refs: #1901

@jasnell jasnell added child_process Issues and PRs related to the child_process subsystem. semver-minor PRs that contain new features and should be released in the next minor version. labels Apr 3, 2016
@bnoordhuis
Copy link
Member

bnoordhuis commented Apr 4, 2016

TBH, it seems kind of pointless to me - most child processes will simply ignore it - and it's already perfectly possible to set an environment variable. I don't really see why this should be in core.

@jasnell
Copy link
Member Author

jasnell commented Apr 4, 2016

Fair enough. What do you think about the docs change, however?

@bnoordhuis
Copy link
Member

bnoordhuis commented Apr 5, 2016

The change to the documentation LGTM if you remove the reference to NODE_CHILD_MAX_BUFFER.

@jasnell jasnell force-pushed the child_process_max_buffer branch from ba28b95 to b693bab Compare April 9, 2016 03:26
@jasnell jasnell changed the title child_process: add NODE_CHILD_MAX_BUFFER env variable doc: clarification for maxBuffer and Unicode output Apr 9, 2016
@jasnell jasnell added doc Issues and PRs related to the documentations. and removed semver-minor PRs that contain new features and should be released in the next minor version. labels Apr 9, 2016
@jasnell jasnell force-pushed the child_process_max_buffer branch from b693bab to 1f02970 Compare April 9, 2016 03:33
@jasnell
Copy link
Member Author

jasnell commented Apr 9, 2016

@bnoordhuis ... updated so that it's just the documentation changes. Also cleaned up some errant line-wrapping issues and end-of-line-whitespace. PTAL

@bnoordhuis
Copy link
Member

bnoordhuis commented Apr 9, 2016

LGTM

1 similar comment
@benjamingr
Copy link
Member

benjamingr commented Apr 10, 2016

LGTM

@jasnell
doc: clarification for maxBuffer and Unicode output
b968cc3 
Clarify caveats on `maxBuffer` with regards to Unicode output.

Refs: nodejs#1901
@jasnell jasnell force-pushed the child_process_max_buffer branch from 1f02970 to b968cc3 Compare April 10, 2016 23:16
jasnell added a commit that referenced this pull request Apr 10, 2016
@jasnell
doc: clarification for maxBuffer and Unicode output
ad2df3a 
Clarify caveats on `maxBuffer` with regards to Unicode output.

Refs: #1901
PR-URL: #6030
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
@jasnell
Copy link
Member Author

jasnell commented Apr 10, 2016

Landed in ad2df3a

@jasnell jasnell closed this Apr 10, 2016
@MylesBorins MylesBorins mentioned this pull request Apr 11, 2016
Closed
@MylesBorins
Copy link
Contributor

MylesBorins commented Apr 11, 2016

@jasnell lts?

MylesBorins pushed a commit that referenced this pull request Apr 19, 2016
@jasnell
doc: clarification for maxBuffer and Unicode output
660de1d 
Clarify caveats on `maxBuffer` with regards to Unicode output.

Refs: #1901
PR-URL: #6030
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
MylesBorins pushed a commit that referenced this pull request Apr 20, 2016
@jasnell
doc: clarification for maxBuffer and Unicode output
79d612f 
Clarify caveats on `maxBuffer` with regards to Unicode output.

Refs: #1901
PR-URL: #6030
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Apr 20, 2016
Closed
MylesBorins pushed a commit that referenced this pull request Apr 20, 2016
@jasnell
doc: clarification for maxBuffer and Unicode output
daaad47 
Clarify caveats on `maxBuffer` with regards to Unicode output.

Refs: #1901
PR-URL: #6030
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
This was referenced Apr 21, 2016
Closed
Closed
@MylesBorins
Copy link
Contributor

MylesBorins commented Apr 21, 2016

@jasnell lts?

@jasnell
Copy link
Member Author

jasnell commented Apr 21, 2016

+1
On Apr 20, 2016 11:33 PM, "Myles Borins" notifications@github.com wrote:

@jasnell https://github.com/jasnell lts?

—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#6030 (comment)

jasnell added a commit that referenced this pull request Apr 26, 2016
@jasnell
doc: clarification for maxBuffer and Unicode output
aa1220d 
Clarify caveats on `maxBuffer` with regards to Unicode output.

Refs: #1901
PR-URL: #6030
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
@MylesBorins
Copy link
Contributor

MylesBorins commented May 17, 2016

@jasnell not landing cleanly, would you like to backport?

@jasnell
Copy link
Member Author

jasnell commented May 17, 2016

will do

@MylesBorins
Copy link
Contributor

MylesBorins commented Aug 30, 2016

ping @jasnell

@jasnell
Copy link
Member Author

jasnell commented Aug 30, 2016

Thanks for the reminder, will do this week.

@jasnell jasnell self-assigned this Aug 30, 2016
@MylesBorins
Copy link
Contributor

MylesBorins commented Nov 18, 2016

@jasnell added don't land. Feel free to still open a backport PR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

@jasnell jasnell

Labels

child_process Issues and PRs related to the child_process subsystem. doc Issues and PRs related to the documentations.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@jasnell @bnoordhuis @benjamingr @MylesBorins