Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

inspector: migrate to internal/errors #15619

Closed
wants to merge 3 commits into from

Conversation

@jasnell
Copy link
Member

@jasnell jasnell commented Sep 26, 2017

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
Copy link
Member Author

@jasnell jasnell commented Sep 26, 2017

@lpinca
lpinca approved these changes Sep 26, 2017
lib/inspector.js Outdated
throw new TypeError(
`"params" must be an object, got ${typeof params} instead`);
throw new errors.TypeError('ERR_INVALID_ARG_TYPE',
'params', 'Object', params);

This comment has been minimized.

@BridgeAR

BridgeAR Sep 26, 2017
Member

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

@BridgeAR
Copy link
Member

@BridgeAR BridgeAR commented Sep 26, 2017

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

@jasnell
Copy link
Member Author

@jasnell jasnell commented Sep 26, 2017

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

@BridgeAR
Copy link
Member

@BridgeAR BridgeAR commented Sep 28, 2017

@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
Copy link
Member Author

@jasnell jasnell commented Sep 28, 2017

Sure :-) gimme a couple of days tho.

@jasnell jasnell mentioned this pull request Sep 28, 2017
79 of 80 tasks complete
@jasnell jasnell force-pushed the jasnell:inspector-internal-errors branch Oct 2, 2017
@jasnell
Copy link
Member Author

@jasnell jasnell commented Oct 2, 2017

Test added!

@jasnell jasnell force-pushed the jasnell:inspector-internal-errors branch 2 times, most recently Oct 2, 2017
@jasnell jasnell requested a review from nodejs/tsc Oct 2, 2017
@jasnell jasnell force-pushed the jasnell:inspector-internal-errors branch to 3498510 Oct 13, 2017
@jasnell
Copy link
Member Author

@jasnell jasnell commented Oct 13, 2017

Ping @nodejs/tsc ... review please?

Copy link
Contributor

@ofrobots ofrobots left a comment

LGTM, Thanks!

@@ -236,6 +236,10 @@ E('ERR_HTTP_INVALID_STATUS_CODE',
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');

This comment has been minimized.

@joyeecheung

joyeecheung Oct 14, 2017
Member

Nit: The inspector is already connected?

jasnell added a commit that referenced this pull request Oct 16, 2017
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
Copy link
Member Author

@jasnell jasnell commented Oct 16, 2017

Landed in 4cf56ad

@jasnell jasnell closed this Oct 16, 2017
addaleax added a commit to ayojs/ayo that referenced this pull request Oct 18, 2017
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants
You can’t perform that action at this time.