Skip to content

errors socket_list : Migrate errors to internal/errors.js#11356

Closed
bougarfaoui wants to merge 3 commits intonodejs:masterfrom
bougarfaoui:internal-errors-socket_list
Closed

errors socket_list : Migrate errors to internal/errors.js#11356
bougarfaoui wants to merge 3 commits intonodejs:masterfrom
bougarfaoui:internal-errors-socket_list

Conversation

@bougarfaoui
Copy link

@bougarfaoui bougarfaoui commented Feb 13, 2017 •
edited by refack
Loading

  • Assign codes to errors reported by internal/socket_list.js

Ref: #11273

Semver-major because this updates specific error messages and converts errors over to use the new internal/errors.js mechanism.

cc @jasnell

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines
Affected core subsystem(s)

errors, socket_list

@nodejs-github-bot nodejs-github-bot added errors Issues and PRs related to JavaScript errors originated in Node.js core. net Issues and PRs related to the net subsystem. labels Feb 13, 2017
jasnell
jasnell requested changes Feb 13, 2017
View reviewed changes
lib/internal/socket_list.js Outdated
Copy link
Member

@jasnell jasnell Feb 13, 2017

Choose a reason for hiding this comment

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

Please use the naming pattern ERR_***

@bougarfaoui
Copy link
Author

bougarfaoui commented Feb 14, 2017

@jasnell requested changes are done.

@jasnell jasnell mentioned this pull request Feb 14, 2017
Closed
80 tasks
jasnell
jasnell reviewed Feb 14, 2017
View reviewed changes
lib/internal/errors.js Outdated
Copy link
Member

@jasnell jasnell Feb 14, 2017

Choose a reason for hiding this comment

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

nit: running make lint should complain on this. There needs to be a single space after the comma before 'Slave...

jasnell
jasnell approved these changes Feb 14, 2017
View reviewed changes
Copy link
Member

@jasnell jasnell 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 a nit

joyeecheung
joyeecheung reviewed Feb 14, 2017
View reviewed changes
lib/internal/socket_list.js Outdated
Copy link
Member

@joyeecheung joyeecheung Feb 14, 2017

Choose a reason for hiding this comment

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

Are there other PR documenting this error code? Otherwise this needs a documentation.

Copy link
Member

@joyeecheung joyeecheung Feb 14, 2017

Choose a reason for hiding this comment

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

Refs: Documenting new errors

Copy link
Author

@bougarfaoui bougarfaoui Feb 14, 2017

Choose a reason for hiding this comment

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

not yet

@jasnell jasnell added the semver-major PRs that contain breaking changes and should be released in the next major version. label Feb 14, 2017
@jasnell jasnell added the blocked PRs that are blocked by other issues or PRs. label Apr 5, 2017
@fhinkel
Copy link
Member

fhinkel commented May 23, 2017

@bougarfaoui Do you want to take a stab at addressing the comments? Thanks!

@fhinkel fhinkel added the stalled Issues and PRs that are stalled. label Jun 7, 2017
@fhinkel
Copy link
Member

fhinkel commented Jun 28, 2017

I'm closing this because it's been inactive for quite a while. Feel free to reopen or ping a collaborator to get it reopened if needed.

@fhinkel fhinkel closed this Jun 28, 2017
@refack
Copy link
Contributor

refack commented Jul 19, 2017

I'm following up

@refack refack reopened this Jul 19, 2017
@refack refack force-pushed the internal-errors-socket_list branch 2 times, most recently from 7cc85e2 to 892ff1c Compare July 19, 2017 21:58
@refack refack force-pushed the internal-errors-socket_list branch from 892ff1c to e4f04f5 Compare July 19, 2017 22:02
@refack refack force-pushed the internal-errors-socket_list branch from e4f04f5 to 71cb153 Compare July 19, 2017 23:08
@refack refack removed blocked PRs that are blocked by other issues or PRs. stalled Issues and PRs that are stalled. labels Jul 19, 2017
@refack refack self-assigned this Jul 19, 2017
@refack
Copy link
Contributor

refack commented Jul 19, 2017

@joyeecheung @fhinkel PTAL ( @Trott ? )

@refack
Copy link
Contributor

refack commented Jul 19, 2017

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

Trott
Trott reviewed Jul 20, 2017
View reviewed changes
doc/api/errors.md Outdated
<a id="ERR_CHILD_CLOSED_BEFORE_REPLY"></a>
### ERR_CHILD_CLOSED_BEFORE_REPLY

Used when a child process is closed before the parent got a replay.
Copy link
Member

@Trott Trott Jul 20, 2017

Choose a reason for hiding this comment

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

Typo: replay -> reply

Micronit: got -> received

Trott
Trott approved these changes Jul 20, 2017
View reviewed changes
Copy link
Member

@Trott Trott left a comment

Choose a reason for hiding this comment

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

If CI is green, LGTM once typo is corrected.

refack pushed a commit to refack/node that referenced this pull request Jul 21, 2017
@refack
errors: migrate socket_list to internal/errors
a03d8ce 
PR-URL: nodejs#11356
Refs: nodejs#11273
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@refack
Copy link
Contributor

refack commented Jul 21, 2017

Landed in a03d8ce

@refack refack closed this Jul 21, 2017
@jasnell jasnell mentioned this pull request Oct 30, 2017
Merged
@refack refack removed their assignment Oct 20, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@joyeecheung joyeecheung joyeecheung left review comments

@jasnell jasnell jasnell approved these changes

@Trott Trott Trott approved these changes

Assignees

No one assigned

Labels

errors Issues and PRs related to JavaScript errors originated in Node.js core. net Issues and PRs related to the net subsystem. semver-major PRs that contain breaking changes and should be released in the next major version.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@bougarfaoui @fhinkel @refack @jasnell @Trott @joyeecheung @nodejs-github-bot