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

dns, net, internal/net: migrate to internals/error by sousandrei · Pull Request #11738 · nodejs/node · GitHub

/ node Public

dns, net, internal/net: migrate to internals/error - #11738

Closed
sousandrei wants to merge 3 commits into
nodejs:masterfrom
sousandrei:master
ConversationCommits (3)ChecksFiles changed
Closed

dns, net, internal/net: migrate to internals/error#11738
sousandrei wants to merge 3 commits into
nodejs:masterfrom
sousandrei:master

Conversation

sousandrei commented Mar 8, 2017 •
edited
Loading

Copy link
Copy Markdown
  • Assign codes to dns.js errors
  • Assign codes to net.js errors
  • Assign codes to internal/net.js errors
  • Added respective documentation

Error codes reused and respective PRs:
#11294

  • ERR_INVALID_ARGS
  • ERR_INVALID_ARG_TYPE
  • ERR_INVALID_CALLBACK

#11302

  • ERR_INVALID_PORT (made some alterations to it)

Ref: #11273

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

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, net, dns, doc

nodejs-github-bot added dns Issues and PRs related to the dns subsystem. errors Issues and PRs related to JavaScript errors originated in Node.js core. net Issues and PRs related to the net subsystem. labels Mar 8, 2017
hiroppy added the doc Issues and PRs related to the documentations. label Mar 8, 2017
mscdex reviewed Mar 8, 2017
View reviewed changes
Comment thread lib/internal/errors.js
var msg = `The "${name}" argument must be `;
if (Array.isArray(expected)) {
var len = expected.length;
expected = expected.map((i) => String(i));

mscdex Mar 8, 2017

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

Avoid these functional Array methods for performance.

mscdex reviewed Mar 8, 2017
View reviewed changes
Comment thread lib/internal/errors.js Outdated
msg += `. Received ${actual !== null ? typeof actual : 'null'}`;
}
return msg;
} No newline at end of file

mscdex Mar 8, 2017

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

Missing newline at end of file.

mscdex reviewed Mar 8, 2017
View reviewed changes
Comment thread doc/api/errors.md
#### error.port

* {Number}
* {number}

mscdex Mar 8, 2017 •
edited
Loading

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

These kinds of changes should be in a separate commit.

mscdex reviewed Mar 8, 2017
View reviewed changes
Comment thread doc/api/errors.md
[try-catch]: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/try...catch
[V8's stack trace API]: https://github.com/v8/v8/wiki/Stack-Trace-API
[vm]: vm.html
[vm]: vm.html No newline at end of file

mscdex Mar 8, 2017

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

What's this change?

mscdex reviewed Mar 8, 2017
View reviewed changes
Comment thread doc/api/errors.md
not capture any frames.

### error.message
#### error.code

mscdex Mar 8, 2017

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

Why is this being duplicated? error.code already exists below.

mscdex reviewed Mar 8, 2017
View reviewed changes
Comment thread lib/dns.js
family = 0;
} else if (typeof callback !== 'function') {
throw new TypeError('Invalid arguments: callback must be passed');
// throw new TypeError('Invalid arguments: callback must be passed');

mscdex Mar 8, 2017

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

This needs to be removed.

mscdex reviewed Mar 8, 2017
View reviewed changes
Comment thread lib/dns.js
err === uv.UV_EAI_NODATA ||
err === uv.UV_EAI_NONAME) {
err === uv.UV_EAI_NODATA ||
err === uv.UV_EAI_NONAME) {

mscdex Mar 8, 2017

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

This should be reverted, the conditionals should be lined up.

mscdex reviewed Mar 8, 2017
View reviewed changes
Comment thread lib/dns.js
hints !== cares.AI_V4MAPPED &&
hints !== (cares.AI_ADDRCONFIG | cares.AI_V4MAPPED)) {
throw new TypeError('Invalid argument: hints must use valid flags');
hints !== cares.AI_ADDRCONFIG &&

mscdex Mar 8, 2017

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

Ditto about reverting the indentation changes.

mscdex reviewed Mar 8, 2017
View reviewed changes
Comment thread lib/internal/errors.js
(err, servers) => `c-ares failed to set servers: "${err}" [${servers}]`);
// Add new errors from here...

// Errors from 111294, port error from 11302

mscdex Mar 8, 2017 •
edited
Loading

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

I think 111294 here should be 11294 instead? Also it's a little bit vague about what this comment is referring to, perhaps it can just be removed?

mscdex reviewed Mar 8, 2017
View reviewed changes
Comment thread lib/internal/errors.js
var msg = `The "${name}" argument must be `;
if (Array.isArray(expected)) {
var len = expected.length;
expected = expected.map((i) => String(i));

mscdex Mar 8, 2017

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

Ditto about Array methods.

mscdex reviewed Mar 8, 2017
View reviewed changes
Comment thread lib/net.js

if (options.lookup && typeof options.lookup !== 'function')
throw new TypeError('"lookup" option should be a function');
// throw new TypeError('"lookup" option should be a function');

mscdex Mar 8, 2017

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

This needs to be removed.

mscdex added the semver-major PRs that contain breaking changes and should be released in the next major version. label Mar 8, 2017

sam-github commented Mar 8, 2017

Copy link
Copy Markdown
Contributor

the other migrations were done module by module, perhaps this should be two PRs, dns: ... and net: ...? Either way, internals/net isn't a sub-system, its net: .

Also, the first commit doesn't follow git commit format guidelines, and the doc change has to be a different PR, its not semver-major, putting it in this PR means it won't be backported.

sousandrei commented Mar 8, 2017

Copy link
Copy Markdown
Author

@sam-github: i was working on dns: and it broke some tests on net: so i ended up migrating that too.
should i fix in this PR the things pointed out by @mscdex in this PR or open a new one?

about the docs, i should remove from this PR and create a new one dedicated to it, right?

sam-github commented Mar 8, 2017

Copy link
Copy Markdown
Contributor

@jasnell was leading the errors stuff, he should weigh in. My suggestion would be to

  • open an entirely new PR for the docs commit, and remove the docs commit from this PR
  • fix the things @mscdex pointed out
  • split this PR (minus the docs commit, of course) into two commits, the first one for net and the second for dns (assuming that dns depends on net, but net doesn't depend on dns)

if dns and net are so intertwined they have to be updated together, then one commit dns, net: .... would be fine instead of seperate net: ... and dns: ... commits.

jasnell commented Mar 8, 2017

Copy link
Copy Markdown
Member

Please separate the internal errors stuff into a separate pr. I'll be tackling those next week

sousandrei commented Mar 8, 2017

Copy link
Copy Markdown
Author

@jasnell can you clarify how should i proceed with this?
Is there a problem making a commit for dns: then one for net:, after that one for the tests and other for the doc?

i was going to separate the doc from the PR but every other PR in the migration have docs with it, so I am confused

About separating internal errors stuff in another PR, you mean internal/net? Even if it breaks the tests?

jasnell added the blocked PRs that are blocked by other issues or PRs. label Apr 5, 2017

fhinkel commented Jun 7, 2017

Copy link
Copy Markdown
Contributor

@sousandrei Can you rebase?

Tests should always work after in every single commit. Documentation changes should go with the relevant code changes in 1 PR. PRs that address a different problem or are a separate refactoring should be in a separate PR.

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

fhinkel commented Jun 28, 2017

Copy link
Copy Markdown
Contributor

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 closed this Jun 28, 2017
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

1 more reviewer
mscdex mscdex left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

blocked PRs that are blocked by other issues or PRs. dns Issues and PRs related to the dns subsystem. doc Issues and PRs related to the documentations. 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. stalled Issues and PRs that are stalled.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

8 participants

Back | FazBrowse Home | New Git URL