Skip to content

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

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

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

Conversation

@bougarfaoui

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

Copy link
Copy Markdown
  • 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-botnodejs-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
Comment threadlib/internal/socket_list.js Outdated

@jasnelljasnellFeb 13, 2017

Copy link
Copy Markdown
Member

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

bougarfaoui commented Feb 14, 2017

Copy link
Copy Markdown
Author

@jasnell requested changes are done.

@jasnelljasnell mentioned this pull request Feb 14, 2017
Closed
80 tasks
jasnell
jasnell reviewed Feb 14, 2017
View reviewed changes
Comment threadlib/internal/errors.js Outdated

@jasnelljasnellFeb 14, 2017

Copy link
Copy Markdown
Member

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

@jasnelljasnell left a comment

Copy link
Copy Markdown
Member

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
Comment threadlib/internal/socket_list.js Outdated

@joyeecheungjoyeecheungFeb 14, 2017

Copy link
Copy Markdown
Member

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.

@joyeecheungjoyeecheungFeb 14, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Refs: Documenting new errors

@bougarfaouibougarfaouiFeb 14, 2017

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

not yet

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

fhinkel commented May 23, 2017

Copy link
Copy Markdown
Member

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

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

fhinkel commented Jun 28, 2017

Copy link
Copy Markdown
Member

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.

@fhinkelfhinkel closed this Jun 28, 2017
@refack

refack commented Jul 19, 2017

Copy link
Copy Markdown
Contributor

I'm following up

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

refack commented Jul 19, 2017

Copy link
Copy Markdown
Contributor

@joyeecheung@fhinkel PTAL ( @Trott ? )

@refack

refack commented Jul 19, 2017

Copy link
Copy Markdown
Contributor

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

Trott
Trott reviewed Jul 20, 2017
View reviewed changes
Comment threaddoc/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.

@TrottTrottJul 20, 2017

Copy link
Copy Markdown
Member

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

@TrottTrott left a comment

Copy link
Copy Markdown
Member

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
Bougarfaoui El houcine@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

refack commented Jul 21, 2017

Copy link
Copy Markdown
Contributor

Landed in a03d8ce

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

Reviewers

@joyeecheungjoyeecheungjoyeecheung left review comments
@jasnelljasnelljasnell approved these changes
@TrottTrottTrott approved these changes

Assignees

No one assigned

Labels

errorsIssues and PRs related to JavaScript errors originated in Node.js core.netIssues and PRs related to the net subsystem.semver-majorPRs 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