Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Appearance settings

doc: clarification for maxBuffer and Unicode output #6030

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
jasnell wants to merge 1 commit into from
Closed

doc: clarification for maxBuffer and Unicode output #6030

jasnell wants to merge 1 commit into from

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
Morty Proxy This is a proxified and sanitized view of the page, visit original site.