Skip to content

Conversation

@devsnek
Copy link
Member

@devsnekdevsnek commented Nov 21, 2017

this was all i could find with ack and a bunch of not nice phrases and words

Checklist
Affected core subsystem(s)

@nodejs-github-botnodejs-github-bot added the test Issues and PRs related to the tests. label Nov 21, 2017
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: maybe use a colon here?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed, colon will yield a cleaner message

@mscdexmscdex added child_process Issues and PRs related to the child_process subsystem. process Issues and PRs related to the process subsystem. repl Issues and PRs related to the REPL subsystem. net Issues and PRs related to the net subsystem. labels Nov 21, 2017
Copy link
Contributor

@apapirovskiapapirovski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM with the nit from @aqrln fixed, also the commit message would ideally be updated to something like test: clean up inappropriate language or something.

@devsnekdevsnekforce-pushed the words branch 2 times, most recently from fff8773 to 6e9b431CompareNovember 21, 2017 14:40
returnparent();
default:
thrownewError(`wtf?${process.argv[2]}`);
thrownewError(`invalid;${process.argv[2]}`);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for being pedantic, but this is a semicolon, not a colon (:) 😄

Copy link
Member

@mhdawsonmhdawson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@jasnell
Copy link
Member

@joyeecheungjoyeecheung added the fast-track PRs that do not need to wait for 48 hours to land. label Nov 21, 2017
@joyeecheung
Copy link
Member

Added fast-track although there are already a ton of approvals.

@joyeecheungjoyeecheung added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Nov 21, 2017
@aqrln
Copy link
Contributor

+1 to fast-tracking. I went ahead and fixed the nit myself. Gonna land it after the CI finishes green.

New CI: https://ci.nodejs.org/job/node-test-pull-request/11623/

@aqrln
Copy link
Contributor

Landed in 7fd8a21.

@aqrlnaqrln closed this Nov 22, 2017
aqrln pushed a commit that referenced this pull request Nov 22, 2017
PR-URL: #17170 Reviewed-By: Alexey Orlenko <[email protected]> Reviewed-By: Gireesh Punathil <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Timothy Gu <[email protected]> Reviewed-By: Franziska Hinkelmann <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Daniel Bevenius <[email protected]> Reviewed-By: Anatoli Papirovski <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Michael Dawson <[email protected]>
@aqrlnaqrln removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Nov 22, 2017
@devsnekdevsnek deleted the words branch November 22, 2017 15:05
MylesBorins pushed a commit that referenced this pull request Dec 12, 2017
PR-URL: #17170 Reviewed-By: Alexey Orlenko <[email protected]> Reviewed-By: Gireesh Punathil <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Timothy Gu <[email protected]> Reviewed-By: Franziska Hinkelmann <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Daniel Bevenius <[email protected]> Reviewed-By: Anatoli Papirovski <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Michael Dawson <[email protected]>
MylesBorins pushed a commit that referenced this pull request Dec 12, 2017
PR-URL: #17170 Reviewed-By: Alexey Orlenko <[email protected]> Reviewed-By: Gireesh Punathil <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Timothy Gu <[email protected]> Reviewed-By: Franziska Hinkelmann <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Daniel Bevenius <[email protected]> Reviewed-By: Anatoli Papirovski <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Michael Dawson <[email protected]>
@MylesBorinsMylesBorins mentioned this pull request Dec 12, 2017
MylesBorins pushed a commit that referenced this pull request Dec 19, 2017
PR-URL: #17170 Reviewed-By: Alexey Orlenko <[email protected]> Reviewed-By: Gireesh Punathil <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Timothy Gu <[email protected]> Reviewed-By: Franziska Hinkelmann <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Daniel Bevenius <[email protected]> Reviewed-By: Anatoli Papirovski <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Michael Dawson <[email protected]>
gibfahn pushed a commit that referenced this pull request Dec 19, 2017
PR-URL: #17170 Reviewed-By: Alexey Orlenko <[email protected]> Reviewed-By: Gireesh Punathil <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Timothy Gu <[email protected]> Reviewed-By: Franziska Hinkelmann <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Daniel Bevenius <[email protected]> Reviewed-By: Anatoli Papirovski <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Michael Dawson <[email protected]>
@gibfahngibfahn mentioned this pull request Dec 20, 2017
gibfahn pushed a commit that referenced this pull request Dec 20, 2017
PR-URL: #17170 Reviewed-By: Alexey Orlenko <[email protected]> Reviewed-By: Gireesh Punathil <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Timothy Gu <[email protected]> Reviewed-By: Franziska Hinkelmann <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Daniel Bevenius <[email protected]> Reviewed-By: Anatoli Papirovski <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Michael Dawson <[email protected]>
@gibfahngibfahn mentioned this pull request Dec 20, 2017
@MylesBorinsMylesBorins mentioned this pull request Dec 20, 2017
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

child_processIssues and PRs related to the child_process subsystem.fast-trackPRs that do not need to wait for 48 hours to land.netIssues and PRs related to the net subsystem.processIssues and PRs related to the process subsystem.replIssues and PRs related to the REPL subsystem.testIssues and PRs related to the tests.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

18 participants

@devsnek@jasnell@joyeecheung@aqrln@apapirovski@fhinkel@danbev@lpinca@TimothyGu@targos@cjihrig@gireeshpunathil@mhdawson@davisjam@mscdex@MylesBorins@gibfahn@nodejs-github-bot