url: fix error message of url.format#11162
Conversation
nodejs-github-bot added the url 
There was a problem hiding this comment.
Probably add a string and a symbol.
There was a problem hiding this comment.
@joyeecheung Since a string is a valid input, so i just added the case of symbol :)
joyeecheung added the semver-major 
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 |
DavidCai1111force-pushed the issue/url-format-error-message branch from 6d3a48d to 9d73e91Compare
joyeecheung commented Feb 4, 2017
@mscdex I'm open to both. Added the label just to be safe. |
addaleax left a comment
addaleax left a comment There was a problem hiding this comment.
+1 to considering this semver-patch
There was a problem hiding this comment.
Can you align the opening ` with the opening '?
DavidCai1111force-pushed the issue/url-format-error-message branch from 9d73e91 to 753c3e3Comparejoyeecheung removed the semver-major 
There was a problem hiding this comment.
Nit: throwsObjsAndReportTypes can be used directly instead of calling entries().
DavidCai1111force-pushed the issue/url-format-error-message branch from 753c3e3 to 8772567Compare
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 commented Feb 4, 2017
mhdawson left a comment
mhdawson left a comment There was a problem hiding this comment.
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
jasnell added the semver-major 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. |
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 commented Feb 11, 2017
Landed in 7818245 |
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]>
12 participants
The previous code:
'Parameter "urlObj" must be an object, not ' + obj === null ? 'null' : typeof objactually equals('Parameter "urlObj" must be an object, not ' + obj) === null ? 'null' : typeof objand 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), orvcbuild test(Windows) passesAffected core subsystem(s)
url