url, test: fix typo in inspect output, add test#10231
Uh oh!
There was an error while loading. Please reload this page.
Conversation
nodejs-github-bot added the url 
jaybrownleeforce-pushed the fix-typo-in-url branch from 1802acb to ed7c570Compare
There was a problem hiding this comment.
Awesome! This LGTM if CI is green and @jasnell and/or @joyeecheung are happy with it
joyeecheung commented Dec 13, 2016
Got a surprised ping here. Can I review this even if I am not a collaborator? |
addaleax commented Dec 13, 2016
@joyeecheung Don’t feel obligated to do anything here! It’s just that I’d feel more comfortable with somebody reviewing this who has a bit more experience with the And to answer your question, yes, you can – PRs still need to be approved by at least one collaborator, but that does not mean that input from others can’t just be as valuable. |
There was a problem hiding this comment.
Nits: Would you mind changing this to assert.strictEqual(keys.has(k), false, 'duplicate key found: ' + k)? Just like what you did on L195.
jaybrownleeforce-pushed the fix-typo-in-url branch from ed7c570 to cf1307fCompare
jaybrownlee commented Dec 13, 2016
@joyeecheung Good suggestion. I made the change, PTAL. |
addaleax commented Dec 14, 2016
One final thing: Could you wrap the commit message body at 72 columns? Otherwise this should be ready to land, and if you prefer, the person landing this can take care of that for you. |
In the string returned from URL.inspect there was an extra semicolon at the end when showHidden === true. The semicolon has been removed and a test for the inspect function has been added. The test parses the returned string, validates all of the contained keys/values and tests logic related to the showHidden option.
jaybrownleeforce-pushed the fix-typo-in-url branch from cf1307f to 35fd1e6Comparejaybrownlee commented Dec 14, 2016
Commit message fixed, wrapped at 72. |
addaleax commented Dec 14, 2016
Great, thanks! Landed in 4f97a14 🎉 |
In the string returned from URL.inspect there was an extra semicolon at the end when showHidden === true. The semicolon has been removed and a test for the inspect function has been added. The test parses the returned string, validates all of the contained keys/values and tests logic related to the showHidden option. PR-URL: nodejs#10231 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Evan Lucas <[email protected]>
In the string returned from URL.inspect there was an extra semicolon at the end when showHidden === true. The semicolon has been removed and a test for the inspect function has been added. The test parses the returned string, validates all of the contained keys/values and tests logic related to the showHidden option. PR-URL: #10231 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Evan Lucas <[email protected]>
7 participants
Checklist
make -j4 test(UNIX), orvcbuild test nosign(Windows) passesAffected core subsystem(s)
Description of change
In the string returned from URL.inspect there was an extra semicolon
at the end when showHidden === true. The semicolon has been
removed and a test for the inspect function has been added. The test
parses the returned string, validates all of the contained keys/values
and tests logic related to the showHidden option.