Skip to content

url: fix error message of url.format#11162

Closed
DavidCai1111 wants to merge 1 commit intonodejs:masterfrom
DavidCai1111:issue/url-format-error-message
Closed

url: fix error message of url.format#11162
DavidCai1111 wants to merge 1 commit intonodejs:masterfrom
DavidCai1111:issue/url-format-error-message

Conversation

@DavidCai1111
Copy link
Member

@DavidCai1111 DavidCai1111 commented Feb 4, 2017 •
edited
Loading

The previous code: 'Parameter "urlObj" must be an object, not ' + obj === null ? 'null' : typeof obj actually equals ('Parameter "urlObj" must be an object, not ' + obj) === null ? 'null' : typeof obj and its former test did not check the error message.

So the eventual error message we got is very weird, like: TypeError: undefined.

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

@nodejs-github-bot nodejs-github-bot added the url Issues and PRs related to the legacy built-in url module. label Feb 4, 2017
joyeecheung
joyeecheung approved these changes Feb 4, 2017
View reviewed changes
test/parallel/test-url-format-invalid-input.js Outdated
Copy link
Member

@joyeecheung joyeecheung Feb 4, 2017

Choose a reason for hiding this comment

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

Probably add a string and a symbol.

Copy link
Member Author

@DavidCai1111 DavidCai1111 Feb 4, 2017

Choose a reason for hiding this comment

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

@joyeecheung Since a string is a valid input, so i just added the case of symbol :)

@joyeecheung joyeecheung added the semver-major PRs that contain breaking changes and should be released in the next major version. label Feb 4, 2017
@mscdex
Copy link
Contributor

mscdex commented Feb 4, 2017

This is probably more a bug fix (semver-patch) than a semver-major change since the error was not there to begin with. I can't imagine many people would be checking the error message for 'undefined'. That's my 2 cents anyway.

@DavidCai1111 DavidCai1111 force-pushed the issue/url-format-error-message branch from 6d3a48d to 9d73e91 Compare February 4, 2017 10:32
@joyeecheung
Copy link
Member

joyeecheung commented Feb 4, 2017

@mscdex I'm open to both. Added the label just to be safe.

addaleax
addaleax approved these changes Feb 4, 2017
View reviewed changes
Copy link
Member

@addaleax addaleax left a comment

Choose a reason for hiding this comment

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

+1 to considering this semver-patch

test/parallel/test-url-format-invalid-input.js Outdated
Copy link
Member

@addaleax addaleax Feb 4, 2017

Choose a reason for hiding this comment

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

Can you align the opening ` with the opening '?

Copy link
Member Author

@DavidCai1111 DavidCai1111 Feb 4, 2017

Choose a reason for hiding this comment

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

@addaleax OK, done 👍

@DavidCai1111 DavidCai1111 force-pushed the issue/url-format-error-message branch from 9d73e91 to 753c3e3 Compare February 4, 2017 11:05
@joyeecheung joyeecheung removed the semver-major PRs that contain breaking changes and should be released in the next major version. label Feb 4, 2017
lpinca
lpinca reviewed Feb 4, 2017
View reviewed changes
test/parallel/test-url-format-invalid-input.js Outdated
Copy link
Member

@lpinca lpinca Feb 4, 2017

Choose a reason for hiding this comment

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

Nit: throwsObjsAndReportTypes can be used directly instead of calling entries().

@DavidCai1111 DavidCai1111 force-pushed the issue/url-format-error-message branch from 753c3e3 to 8772567 Compare February 4, 2017 13:39
@jasnell
Copy link
Member

jasnell commented Feb 4, 2017

I'm not as certain. I think it's better to treat all of these in a consistent way. I'd argue semver-major but let's see what the rest of @nodejs/ctc has to say

@TimothyGu
Copy link
Member

TimothyGu commented Feb 4, 2017

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

@thefourtheye thefourtheye mentioned this pull request Feb 6, 2017
Closed
3 tasks
mhdawson
mhdawson approved these changes Feb 7, 2017
View reviewed changes
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, I'd be tempted to be conservative and treat as semver major unless we know it is causing pain to users in current versions.

@jasnell
Copy link
Member

jasnell commented Feb 7, 2017

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

@jasnell jasnell added the semver-major PRs that contain breaking changes and should be released in the next major version. label Feb 11, 2017
@jasnell
Copy link
Member

jasnell commented Feb 11, 2017

I'm going to land this as a semver-major. We can back that back down after the fact if necessary.

jasnell pushed a commit that referenced this pull request Feb 11, 2017
@davidtaikocha @jasnell
url: fix error message of url.format
7818245 
PR-URL: #11162
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Sakthipriyan Vairamani <[email protected]>
Reviewed-By: Timothy Gu <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
@jasnell
Copy link
Member

jasnell commented Feb 11, 2017

Landed in 7818245

@jasnell jasnell closed this Feb 11, 2017
@thefourtheye thefourtheye mentioned this pull request Feb 12, 2017
Closed
krydos pushed a commit to krydos/node that referenced this pull request Feb 25, 2017
@davidtaikocha @krydos
url: fix error message of url.format
f39c7d9 
PR-URL: nodejs#11162
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Sakthipriyan Vairamani <[email protected]>
Reviewed-By: Timothy Gu <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
@jasnell jasnell mentioned this pull request Apr 4, 2017
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jasnell jasnell jasnell approved these changes

@addaleax addaleax addaleax approved these changes

@lpinca lpinca lpinca approved these changes

@cjihrig cjihrig cjihrig approved these changes

@joyeecheung joyeecheung joyeecheung approved these changes

@mhdawson mhdawson mhdawson approved these changes

+2 more reviewers

@thefourtheye thefourtheye thefourtheye approved these changes

@TimothyGu TimothyGu TimothyGu approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

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

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.