url, test: fix typo in inspect output, add test #10231
Conversation
nodejs-github-bot
added
the
url
jaybrownlee
force-pushed
the
fix-typo-in-url
branch
from
1802acb
to
ed7c570
Compare
There was a problem hiding this comment.
Awesome! This LGTM if CI is green and @jasnell and/or @joyeecheung are happy with it
|
Got a surprised ping here. Can I review this even if I am not a collaborator? |
|
@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. |
| const v = line.slice(i + 2); | ||
| if (keys.has(k)) { | ||
| common.fail('duplicate key found: ' + k); | ||
| } |
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.
jaybrownlee
force-pushed
the
fix-typo-in-url
branch
from
ed7c570
to
cf1307f
Compare
|
@joyeecheung Good suggestion. I made the change, PTAL. |
|
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.
jaybrownlee
force-pushed
the
fix-typo-in-url
branch
from
cf1307f
to
35fd1e6
Compare
|
Commit message fixed, wrapped at 72. |
|
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 <anna@addaleax.net> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Evan Lucas <evanlucas@me.com>
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 <anna@addaleax.net> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Evan Lucas <evanlucas@me.com>
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.