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

url: fix error message of url.format by DavidCai1111 · Pull Request #11162 · nodejs/node · GitHub

/ node Public

url: fix error message of url.format - #11162

Closed
DavidCai1111 wants to merge 1 commit into
nodejs:masterfrom
DavidCai1111:issue/url-format-error-message
ConversationCommits (1)ChecksFiles changed
Closed

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

Conversation

DavidCai1111 commented Feb 4, 2017 •
edited
Loading

Copy link
Copy Markdown
Member

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 added the url Issues and PRs related to the legacy built-in url module. label Feb 4, 2017
joyeecheung approved these changes Feb 4, 2017
View reviewed changes
Comment thread test/parallel/test-url-format-invalid-input.js Outdated

joyeecheung Feb 4, 2017

Copy link
Copy Markdown
Member

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

Probably add a string and a symbol.

DavidCai1111 Feb 4, 2017

Copy link
Copy Markdown
Member Author

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

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

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

mscdex commented Feb 4, 2017

Copy link
Copy Markdown
Contributor

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 force-pushed the issue/url-format-error-message branch from 6d3a48d to 9d73e91 Compare February 4, 2017 10:32

joyeecheung commented Feb 4, 2017

Copy link
Copy Markdown
Member

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

addaleax approved these changes Feb 4, 2017
View reviewed changes

addaleax left a comment

Copy link
Copy Markdown
Member

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

+1 to considering this semver-patch

Comment thread test/parallel/test-url-format-invalid-input.js Outdated

addaleax Feb 4, 2017

Copy link
Copy Markdown
Member

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

Can you align the opening ` with the opening '?

DavidCai1111 Feb 4, 2017

Copy link
Copy Markdown
Member Author

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

@addaleax OK, done 👍

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

lpinca Feb 4, 2017

Copy link
Copy Markdown
Member

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

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

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

jasnell commented Feb 4, 2017

Copy link
Copy Markdown
Member

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 commented Feb 4, 2017

Copy link
Copy Markdown
Member

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

thefourtheye mentioned this pull request Feb 6, 2017
Closed
3 tasks
mhdawson approved these changes Feb 7, 2017
View reviewed changes

mhdawson left a comment

Copy link
Copy Markdown
Member

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

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 commented Feb 7, 2017

Copy link
Copy Markdown
Member

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

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

jasnell commented Feb 11, 2017

Copy link
Copy Markdown
Member

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
url: fix error message of url.format
7818245 
PR-URL: #11162
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>

jasnell commented Feb 11, 2017

Copy link
Copy Markdown
Member

Landed in 7818245

jasnell closed this Feb 11, 2017
thefourtheye mentioned this pull request Feb 12, 2017
Closed
krydos pushed a commit to krydos/node that referenced this pull request Feb 25, 2017
url: fix error message of url.format
f39c7d9 
PR-URL: nodejs#11162
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
jasnell mentioned this pull request Apr 4, 2017
Closed
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

jasnell jasnell approved these changes
addaleax addaleax approved these changes
lpinca lpinca approved these changes
cjihrig cjihrig approved these changes
joyeecheung joyeecheung approved these changes
+3 more reviewers
thefourtheye thefourtheye approved these changes
TimothyGu TimothyGu approved these changes
mhdawson mhdawson 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.

Back | FazBrowse Home | New Git URL