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

errors, url: port url errors to internal/errors #13963

Closed
wants to merge 1 commit into from

Conversation

starkwang
Copy link
Contributor

@starkwang starkwang commented Jun 28, 2017

There is already a PR (#11360) for this migration, but it seems to be out-of-date and deprecated. So I make a new one.

Ref: #11273

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines
Affected core subsystem(s)

url, test

@nodejs-github-bot nodejs-github-bot added the url Issues and PRs related to the legacy built-in url module. label Jun 28, 2017
@TimothyGu
Copy link
Member

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

Don't worry if node-test-commit-linux doesn't pass. The CI bot has had some trouble recently.

@refack refack added errors Issues and PRs related to JavaScript errors originated in Node.js core. semver-major PRs that contain breaking changes and should be released in the next major version. labels Jun 29, 2017
code: 'ERR_INVALID_ARG_TYPE',
type: TypeError,
message: 'The "urlObj" argument must be of type object. ' +
`Received type ${type}`
Copy link
Member

Choose a reason for hiding this comment

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

nit: line up the strings here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

});

assert.throws(() => { url.parse('http://%E0%A4%A@fail'); },
/^URIError: URI malformed$/);
assert.throws(() => { url.parse('http://%E0%A4%A@fail'); }, /^URIError: URI malformed$/);
Copy link
Member

Choose a reason for hiding this comment

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

long line here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, I fix it.

Copy link
Member

@mhdawson mhdawson left a comment

Choose a reason for hiding this comment

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

LGTM

@mhdawson
Copy link
Member

refack pushed a commit to refack/node that referenced this pull request Jul 2, 2017
PR-URL: nodejs#13963
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Timothy Gu <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@refack
Copy link
Contributor

refack commented Jul 2, 2017

Landed in 473f0ef

@refack refack closed this Jul 2, 2017
@refack
Copy link
Contributor

refack commented Jul 2, 2017

Extra sanity run on master: https://ci.nodejs.org/job/node-test-commit/10881/

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
errors Issues and PRs related to JavaScript errors originated in Node.js core. semver-major PRs that contain breaking changes and should be released in the next major version. url Issues and PRs related to the legacy built-in url module.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants