Skip to content
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

doc: clarification for maxBuffer and Unicode output #6030

Closed
wants to merge 1 commit into from
Closed

doc: clarification for maxBuffer and Unicode output #6030

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

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

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

LGTM

1 similar comment
@benjamingr
Copy link
Member

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 <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
@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

@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 <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
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 <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
@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 <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
This was referenced Apr 21, 2016
Closed
Closed
@MylesBorins
Copy link
Contributor

@jasnell lts?

@jasnell
Copy link
Member Author

jasnell commented Apr 21, 2016

+1
On Apr 20, 2016 11:33 PM, "Myles Borins" [email protected] 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 <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
@MylesBorins
Copy link
Contributor

@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

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

@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