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

inspector: migrate to internal/errors by jasnell · Pull Request #15619 · nodejs/node · GitHub

/ node Public

inspector: migrate to internal/errors - #15619

Closed
jasnell wants to merge 3 commits into
nodejs:masterfrom
jasnell:inspector-internal-errors
ConversationCommits (3)ChecksFiles changed
Closed

inspector: migrate to internal/errors#15619
jasnell wants to merge 3 commits into
nodejs:masterfrom
jasnell:inspector-internal-errors

Conversation

jasnell commented Sep 26, 2017

Copy link
Copy Markdown
Member

migrate to internal/errors

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)

inspector,errors

jasnell added errors Issues and PRs related to JavaScript errors originated in Node.js core. inspector Issues and PRs related to the V8 inspector protocol labels Sep 26, 2017
nodejs-github-bot added errors Issues and PRs related to JavaScript errors originated in Node.js core. inspector Issues and PRs related to the V8 inspector protocol labels Sep 26, 2017

jasnell commented Sep 26, 2017

Copy link
Copy Markdown
Member Author

Ping @nodejs/tsc

jasnell commented Sep 26, 2017

Copy link
Copy Markdown
Member Author

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

jasnell added the semver-major PRs that contain breaking changes and should be released in the next major version. label Sep 26, 2017
BridgeAR reviewed Sep 26, 2017
View reviewed changes
Comment thread lib/inspector.js Outdated

BridgeAR Sep 26, 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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Other errors mainly use 'object' (lower cased).

BridgeAR commented Sep 26, 2017 •
edited
Loading

Copy link
Copy Markdown
Member

Do we not have any tests for the inspector error cases?

jasnell commented Sep 26, 2017

Copy link
Copy Markdown
Member Author

@BridgeAR ... none that were affected... which is a bit scary.

BridgeAR commented Sep 28, 2017

Copy link
Copy Markdown
Member

@jasnell would you be so kind and add some for those? I think it would be a shame not to use the opportunity otherwise.

jasnell commented Sep 28, 2017

Copy link
Copy Markdown
Member Author

Sure :-) gimme a couple of days tho.

jasnell mentioned this pull request Sep 28, 2017
Closed
80 tasks
jasnell force-pushed the inspector-internal-errors branch from 6aac4f2 to 81cf213 Compare October 2, 2017 18:43

jasnell commented Oct 2, 2017

Copy link
Copy Markdown
Member Author

Test added!

jasnell force-pushed the inspector-internal-errors branch 2 times, most recently from 6c1c982 to 5bf3c97 Compare October 2, 2017 19:12
jasnell requested a review from a team October 2, 2017 22:55
jasnell force-pushed the inspector-internal-errors branch from 5bf3c97 to 3498510 Compare October 13, 2017 21:07

jasnell commented Oct 13, 2017

Copy link
Copy Markdown
Member Author

Ping @nodejs/tsc ... review please?

jasnell commented Oct 13, 2017

Copy link
Copy Markdown
Member Author

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

ofrobots approved these changes Oct 13, 2017
View reviewed changes

ofrobots left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

LGTM, Thanks!

joyeecheung approved these changes Oct 14, 2017
View reviewed changes
Comment thread lib/internal/errors.js Outdated
E('ERR_HTTP_TRAILER_INVALID',
'Trailers are invalid with this transfer encoding');
E('ERR_INDEX_OUT_OF_RANGE', 'Index out of range');
E('ERR_INSPECTOR_ALREADY_CONNECTED', 'Already connected');

joyeecheung Oct 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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Nit: The inspector is already connected?

jasnell added a commit that referenced this pull request Oct 16, 2017
inspector: migrate to internal/errors
4cf56ad 
PR-URL: #15619
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>

jasnell commented Oct 16, 2017

Copy link
Copy Markdown
Member Author

Landed in 4cf56ad

jasnell closed this Oct 16, 2017
addaleax pushed a commit to ayojs/ayo that referenced this pull request Oct 18, 2017
inspector: migrate to internal/errors
efce076 
PR-URL: nodejs/node#15619
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
jasnell mentioned this pull request Oct 30, 2017
Merged
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

BridgeAR BridgeAR left review comments
lpinca lpinca approved these changes
joyeecheung joyeecheung approved these changes
fhinkel Awaiting requested review from fhinkel
addaleax Awaiting requested review from addaleax
mhdawson Awaiting requested review from mhdawson
+1 more reviewer
ofrobots ofrobots approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

errors Issues and PRs related to JavaScript errors originated in Node.js core. inspector Issues and PRs related to the V8 inspector protocol 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.

6 participants

Back | FazBrowse Home | New Git URL