FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

doc: clarification for maxBuffer and Unicode output by jasnell · Pull Request #6030 · nodejs/node · GitHub

/ node Public

doc: clarification for maxBuffer and Unicode output - #6030

Closed
jasnell wants to merge 1 commit into
nodejs:masterfrom
jasnell:child_process_max_buffer
ConversationCommits (1)ChecksFiles changed
Closed

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

Conversation

jasnell commented Apr 3, 2016

Copy link
Copy Markdown
Member

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 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 commented Apr 4, 2016

Copy link
Copy Markdown
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 commented Apr 4, 2016

Copy link
Copy Markdown
Member Author

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

bnoordhuis commented Apr 5, 2016

Copy link
Copy Markdown
Member

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

jasnell force-pushed the child_process_max_buffer branch from ba28b95 to b693bab Compare April 9, 2016 03:26
jasnell changed the title child_process: add NODE_CHILD_MAX_BUFFER env variable doc: clarification for maxBuffer and Unicode output Apr 9, 2016
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 force-pushed the child_process_max_buffer branch from b693bab to 1f02970 Compare April 9, 2016 03:33

jasnell commented Apr 9, 2016

Copy link
Copy Markdown
Member Author

@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 commented Apr 9, 2016

Copy link
Copy Markdown
Member

LGTM

1 similar comment

benjamingr commented Apr 10, 2016

Copy link
Copy Markdown
Member

LGTM

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

Refs: nodejs#1901
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
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 commented Apr 10, 2016

Copy link
Copy Markdown
Member Author

Landed in ad2df3a

jasnell closed this Apr 10, 2016
MylesBorins mentioned this pull request Apr 11, 2016
Closed

MylesBorins commented Apr 11, 2016

Copy link
Copy Markdown
Contributor

@jasnell lts?

MylesBorins pushed a commit that referenced this pull request Apr 19, 2016
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
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 mentioned this pull request Apr 20, 2016
Closed
MylesBorins pushed a commit that referenced this pull request Apr 20, 2016
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 commented Apr 21, 2016

Copy link
Copy Markdown
Contributor

@jasnell lts?

jasnell commented Apr 21, 2016

Copy link
Copy Markdown
Member Author

+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
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 commented May 17, 2016

Copy link
Copy Markdown
Contributor

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

jasnell commented May 17, 2016

Copy link
Copy Markdown
Member Author

will do

MylesBorins commented Aug 30, 2016

Copy link
Copy Markdown
Contributor

ping @jasnell

jasnell commented Aug 30, 2016

Copy link
Copy Markdown
Member Author

Thanks for the reminder, will do this week.

jasnell self-assigned this Aug 30, 2016

MylesBorins commented Nov 18, 2016

Copy link
Copy Markdown
Contributor

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

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

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

Back | FazBrowse Home | New Git URL