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

dns, errors: Migrate to use internal/errors #14212

Closed
wants to merge 1 commit into from

Conversation

@starkwang
Copy link
Contributor

@starkwang starkwang commented Jul 13, 2017

Migrate dns errors to use internal/errors.

Ref: #11273

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)

dns, errors

@starkwang starkwang force-pushed the starkwang:dns-internal-errors branch Jul 13, 2017
doc/api/errors.md Outdated
@@ -580,6 +580,10 @@ by the `assert` module.

Used when attempting to perform an operation outside the bounds of a `Buffer`.

<a id="ERR_CARES_FAILED"></a>

This comment has been minimized.

@jasnell

jasnell Jul 14, 2017
Member

Perhaps ERR_DNS_SET_SERVERS_FAILED?

Copy link
Member

@jasnell jasnell left a comment

Very close! thank for doing this. Left a comment I'd like to see addressed.

@starkwang starkwang force-pushed the starkwang:dns-internal-errors branch 2 times, most recently Jul 15, 2017
Copy link
Member

@jasnell jasnell left a comment

LGTM if CI is green.

Copy link
Member

@XadillaX XadillaX left a comment

LGTM

@starkwang
Copy link
Contributor Author

@starkwang starkwang commented Jul 22, 2017

One failure in CI looks unrelated.
https://ci.nodejs.org/job/node-test-binary-arm/RUN_SUBSET=0,label=pi1-raspbian-wheezy/9392/console

  ...
not ok 23 parallel/test-child-process-fork-exec-path
  ---
  duration_ms: 8.891
  severity: fail
  stack: |-
  ...
@refack
refack approved these changes Jul 24, 2017
Copy link
Member

@refack refack left a comment

Some nits

lib/dns.js Outdated
throw new TypeError('Invalid arguments: ' +
'hostname must be a string or falsey');
throw new errors.TypeError('ERR_INVALID_ARG_TYPE', 'hostname',
['string', 'falsey'], hostname);

This comment has been minimized.

@refack

refack Jul 24, 2017
Member

new indentation rule requires aligning to the call (
#14403

lib/dns.js Outdated
@@ -290,13 +292,14 @@ function resolve(hostname, rrtype, callback) {
resolver = resolveMap.A;
callback = rrtype;
} else {
throw new TypeError('"rrtype" argument must be a string');
throw new errors.TypeError('ERR_INVALID_ARG_TYPE', 'rrtype',
'string', rrtype);

This comment has been minimized.

lib/internal/errors.js Outdated
@@ -113,6 +113,10 @@ E('ERR_BUFFER_OUT_OF_BOUNDS', bufferOutOfBounds);
E('ERR_CONSOLE_WRITABLE_STREAM',
'Console expects a writable stream instance for %s');
E('ERR_CPU_USAGE', 'Unable to obtain cpu usage %s');
E('ERR_DNS_SET_SERVERS_FAILED',
(err, servers) => {

This comment has been minimized.

@refack

refack Jul 24, 2017
Member

probably better (err, servers) => `c-ares failed to set servers: "${err}" [${servers}]`

test/parallel/test-c-ares.js Outdated
// Try calling resolve with an unsupported type that's an object key
'toString'
].forEach((val) => {
assert.throws(() => {

This comment has been minimized.

@refack

refack Jul 24, 2017
Member

New feature of expectsError can be used without assert.throws:

common.expectsError(
  dns.resolve('www.google.com', val),
  {
    code: 'ERR_INVALID_OPT_VALUE',
    type: TypeError,
    message: `The value "${val}" is invalid for option "rrtype"`
  }
);
doc/api/errors.md Outdated
@@ -591,6 +591,10 @@ Used when `Console` is instantiated without `stdout` stream or when `stdout` or

Used when the native call from `process.cpuUsage` cannot be processed properly.

<a id="ERR_DNS_SET_SERVERS_FAILED"></a>

Used when `c-ares` failed to set the server.

This comment has been minimized.

@refack

refack Jul 24, 2017
Member

Used when `c-ares` failed to set the DNS servers.

@jasnell jasnell mentioned this pull request Jul 24, 2017
79 of 80 tasks complete
@refack
Copy link
Member

@refack refack commented Jul 24, 2017

@starkwang starkwang force-pushed the starkwang:dns-internal-errors branch to 0653452 Jul 24, 2017
@starkwang
Copy link
Contributor Author

@starkwang starkwang commented Jul 24, 2017

Pushed commit to address comments.

Copy link
Member

@mcollina mcollina left a comment

LGTM

@refack
Copy link
Member

@refack refack commented Jul 24, 2017

Landed in 9cb390d

@refack refack closed this Jul 24, 2017
refack added a commit that referenced this pull request Jul 24, 2017
PR-URL: #14212
Refs: #11273
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Khaidi Chu <i@2333.moe>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
@starkwang starkwang mentioned this pull request Jul 27, 2017
4 of 4 tasks complete
@refack refack added this to Done in Error Codes Aug 20, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Error Codes
  
Done
Linked issues

Successfully merging this pull request may close these issues.

None yet

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